fix(server): set no-cache headers on private project export downloads (SEC-04) - #2360
Open
wilfredmulenga wants to merge 5 commits into
Open
fix(server): set no-cache headers on private project export downloads (SEC-04)#2360wilfredmulenga wants to merge 5 commits into
wilfredmulenga wants to merge 5 commits into
Conversation
GET /export/:filename streamed a project's export zip with no Cache-Control header, relying entirely on intermediary defaults. The URL is a predictable function of the project ID, so a CDN or shared cache placed in front of this service could store an authorized response and later serve it to a different, unauthenticated requester of the same URL. Apply the same privateCache middleware already used on other sensitive routes, so the response is never cached regardless of what sits in front of the server.
Covers SEC-04: asserts /export/:filename always sets a no-cache Cache-Control header, even on a rejected request, and verified this fails without the privateCache middleware in place.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a potential caching/security gap on the export download endpoint by ensuring export ZIP responses are not cacheable by intermediaries, matching the behavior already used on other sensitive routes.
Changes:
- Adds the existing
privateCachemiddleware toGET /export/:filenameto emitCache-Control: private, no-store, no-cache, must-revalidate. - Adds a regression test to assert the
Cache-Controlheader is set on a rejected request.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/internal/app/export.go | Adds privateCache middleware to the export download route. |
| server/internal/app/export_test.go | Introduces a regression test asserting Cache-Control is set on rejected export requests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Echo composes route middleware so the last one in the list runs closest to the handler. privateCache was listed last, so if optionalAuth short-circuited (e.g. a bad Authorization header triggering a 401 before calling next), the response went out with no Cache-Control header, missing the response class this fix was meant to cover. Move privateCache first so it's outermost and always runs, setting the header on every response regardless of where the chain stops.
wilfredmulenga
changed the title
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
GET /export/:filename streamed a project's export zip with no Cache-Control header, relying entirely on intermediary defaults. The URL is a predictable function of the project ID, so a CDN or shared cache placed in front of this service could store an authorized response and later serve it to a different, unauthenticated requester of the same URL.
Checked the deployed infrastructure: no CDN or shared cache actually sits in front of this service today (enable_cdn is false everywhere it matters), so this isn't an active leak right now. This closes the gap regardless, since it's a real bug independent of the current network setup and cheap to fix.
Fix
Apply the same privateCache middleware already used on other sensitive routes (e.g. /api/graphql), so the response always carries
Cache-Control: private, no-store, no-cache, must-revalidate, regardless of what sits in front of the server.Test plan
go build ./...passes