proxy: Follow multi-hop upstream redirects

.github/workflows/ci.yaml

@@ -39,7 +39,7 @@ jobs:
         with:
           persist-credentials: false
 
-      - uses: reviewdog/action-actionlint@320fcdd9c860767cf17fab3b20e22e739d5d02b8 # v1
+      - uses: reviewdog/action-actionlint@7abf25e1607ec65838b73d0484ff31f8e9499d80 # v1
 
   golangci-lint:
     runs-on: ubuntu-latest

CHANGELOG.md

@@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ## [Unreleased](https://github.com/ganto/pkgproxy/commits/HEAD/)
 
+### Changed
+
+- Reject upstream redirects that downgrade from HTTPS to HTTP
+
+### Fixed
+
+- Follow multi-hop upstream redirects, restoring package caching for Fedora COPR repositories
+
 ## [v0.3.0](https://github.com/ganto/pkgproxy/releases/tag/v0.3.0) - 2026-08-29
 
 ### Added

docs/architecture.md

@@ -23,7 +23,7 @@ The **first path segment** of the URL is the repository name (e.g. `/fedora/...`
 
 ## Mirror Failover & Retry (`tryMirrors`)
 
-Mirrors are tried in order. Per mirror, up to `retries` attempts are made (default 1). Exponential backoff (`retryBaseDelay * 2^(attempt-2)`, starting at 1 s) is triggered only on 5xx responses. A single redirect (301/302/303/307/308) is followed per attempt. Connection-level errors skip immediately to the next mirror. The first 200 response wins; otherwise the last non-nil response is returned.
+Mirrors are tried in order. Per mirror, up to `retries` attempts are made (default 1). Exponential backoff (`retryBaseDelay * 2^(attempt-2)`, starting at 1 s) is triggered only on 5xx responses. Up to 10 redirects (301/302/303/307/308) are followed per attempt; HTTPS → HTTP downgrades are rejected, and an exceeded limit or rejected downgrade marks the mirror as failed. Connection-level errors skip immediately to the next mirror. The first 200 response wins; otherwise the last non-nil response is returned.
 
 ## Cache Write Path
 

openspec/changes/archive/2026-11-13-follow-multi-hop-redirects/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-11-13

openspec/changes/archive/2026-11-13-follow-multi-hop-redirects/design.md

@@ -0,0 +1,38 @@
+## Context
+
+`tryMirrors` (`pkg/pkgproxy/proxy.go`) tries each mirror up to `retries` times. After the initial upstream request, a single `if` checks for a redirect status, follows `Location` once, and then evaluates the response: 200 wins, a 5xx retries, anything else moves on to the next mirror. Fedora COPR now redirects twice (COPR → Red Hat Pulp → S3), so the second `302` falls through and is returned to the client by the handler as-is.
+
+The handler already turns `(nil, err)` from `tryMirrors` into 502 Bad Gateway, so no new error plumbing is needed.
+
+## Goals / Non-Goals
+
+**Goals:**
+- Follow redirect chains of up to 10 hops per mirror attempt.
+- Reject HTTPS → HTTP downgrades at any hop.
+- Treat an unusable redirect chain as a failed mirror and never leak a 3xx to the client.
+
+**Non-Goals:**
+- A configurable hop limit.
+- SSRF hardening (blocking private or loopback redirect targets).
+- Changing method or header semantics on redirect (e.g. 303 → GET).
+- Pointing the `copr` mirror at the Pulp URL directly. The official COPR URL remains the configured entry point.
+
+## Decisions
+
+**Manual loop instead of `http.Client` with `CheckRedirect`.**
+Replace the single `if` with a loop bounded by a `maxRedirects = 10` constant. Before each hop, check the hop count and the scheme transition. It's a small diff that keeps today's semantics: same method and buffered body replayed, headers filtered by `allowedRequestHeaders`, and one `upstream response` log line per hop. *Alternative:* `http.Client` provides the 10-hop default and a `CheckRedirect` hook, but it rewrites 301/302/303 to GET, strips headers on cross-host hops, drops per-hop logging, and requires reworking `forwardClientRequestToOrigin` and the transport tests.
+
+**Limit of 10, as a fixed constant.**
+This matches Go's `http.Client` default, so the behaviour is familiar. COPR needs 2 hops. *Alternative:* 5 (tighter, but no real benefit) or a per-repo config key (adds docs and validation with no use case).
+
+**Downgrade check compares each hop with the previous URL.**
+Reject when the current URL's scheme is `https` and the `Location` scheme is `http`. Because each hop is checked against the one before it, an `https` link anywhere in the chain prevents a later `http` hop. An `http`-configured mirror can still redirect to `http`. The check applies to the first hop too.
+
+**Failure handling reuses the existing connection-error path.**
+Exceeding the limit or rejecting a downgrade closes the response, sets `rsp = nil` and a descriptive `err`, logs a warning, and `break`s to the next mirror. These are not retried, just like a bad `Location`. If the last mirror ends this way, `tryMirrors` returns `(nil, err)` and the handler responds 502.
+
+## Risks / Trade-offs
+
+- [Mirrors that currently redirect from HTTPS to plain HTTP will stop working] → Called out as a separate **Changed** entry in `CHANGELOG.md`. Operators can configure the `http` mirror URL directly if they accept it.
+- [An `http` → `http` → `https` → `http` chain is rejected only at the last hop] → Intended: once on HTTPS, the chain must stay on HTTPS.
+- [Presigned S3 URLs may expire between hops] → Not a concern in practice: hops happen within milliseconds of each other.

openspec/changes/archive/2026-11-13-follow-multi-hop-redirects/proposal.md

@@ -0,0 +1,23 @@
+## Why
+
+Fedora COPR now serves downloads through a two-hop redirect chain (`download.copr.fedorainfracloud.org` → `packages.redhat.com` Pulp → S3). pkgproxy follows only one redirect per mirror attempt, so it passes the second `302` through to the client. The client then downloads directly from upstream and nothing is cached. The `*/COPR` e2e subtests fail on Fedora, CentOS Stream, AlmaLinux, and Rocky Linux.
+
+## What Changes
+
+- Follow up to 10 upstream redirects (301/302/303/307/308) per mirror attempt instead of one.
+- Reject any redirect hop that downgrades from HTTPS to HTTP. **BREAKING** for mirrors that currently redirect from `https` to plain `http`: the mirror is now treated as failed.
+- When a mirror's redirect chain exceeds the limit or is rejected, mark that mirror as failed and try the next one. If every mirror fails, respond with 502 Bad Gateway. The proxy never passes a 3xx response to the client.
+
+## Capabilities
+
+### New Capabilities
+- `upstream-redirects`: how the proxy follows upstream redirects per mirror attempt: hop limit, downgrade protection, and failure handling.
+
+### Modified Capabilities
+
+## Impact
+
+- Code: `tryMirrors` in `pkg/pkgproxy/proxy.go`. Unit tests in `pkg/pkgproxy/proxy_test.go`.
+- Docs: `docs/architecture.md` (mirror/redirect paragraph), `CHANGELOG.md` `[Unreleased]`.
+- No new configuration options, no dependency changes. README is unchanged.
+- The existing COPR e2e subtests serve as the regression check.

openspec/changes/archive/2026-11-13-follow-multi-hop-redirects/specs/upstream-redirects/spec.md

@@ -0,0 +1,44 @@
+## ADDED Requirements
+
+### Requirement: Follow multi-hop upstream redirects
+The proxy SHALL follow upstream redirect responses (301, 302, 303, 307, 308) for each mirror attempt, up to a maximum of 10 redirect hops. Every hop SHALL replay the original request method and buffered body, with the same filtered request headers.
+
+#### Scenario: Two-hop redirect chain is followed and cached
+- **WHEN** a client requests a cacheable `.rpm` file and the mirror redirects A → B → C, where C returns 200
+- **THEN** the proxy returns 200 with C's body to the client
+- **AND** the file is written to the cache
+
+#### Scenario: Single redirect still works
+- **WHEN** the mirror returns a single redirect to a location that returns 200
+- **THEN** the proxy returns 200 with the redirected body
+
+### Requirement: Redirect hop limit
+When a mirror is still redirecting after 10 hops, the proxy SHALL treat that mirror attempt as failed and continue with the next mirror.
+
+#### Scenario: Redirect loop exceeds the limit
+- **WHEN** the only mirror redirects in a loop that never returns a non-redirect response
+- **THEN** the proxy responds with 502 Bad Gateway
+- **AND** the client does not receive a 3xx response
+
+#### Scenario: Fallback to next mirror after the limit is exceeded
+- **WHEN** mirror 1 exceeds the redirect limit and mirror 2 returns 200
+- **THEN** the proxy returns 200 with mirror 2's body
+
+### Requirement: Reject HTTPS to HTTP redirect downgrades
+The proxy SHALL reject any redirect hop from an `https` URL to an `http` location, including the first hop, and treat that mirror attempt as failed. A redirect from an `http` URL to another `http` location SHALL still be followed.
+
+#### Scenario: HTTPS mirror redirects to plain HTTP
+- **WHEN** the only mirror is served over HTTPS and redirects to an `http` location
+- **THEN** the proxy does not request the `http` location
+- **AND** the proxy responds with 502 Bad Gateway
+
+#### Scenario: HTTP mirror redirects to HTTP
+- **WHEN** a mirror configured with an `http` URL redirects to another `http` location that returns 200
+- **THEN** the proxy returns 200 with the redirected body
+
+### Requirement: Never pass redirects to the client
+The proxy SHALL NOT forward an upstream 3xx redirect response to the client. If no mirror yields a usable response, the proxy SHALL respond with 502 Bad Gateway.
+
+#### Scenario: All mirrors fail due to redirect errors
+- **WHEN** every mirror either exceeds the redirect limit or attempts an HTTPS to HTTP downgrade
+- **THEN** the proxy responds with 502 Bad Gateway

openspec/changes/archive/2026-11-13-follow-multi-hop-redirects/tasks.md

@@ -0,0 +1,29 @@
+## 1. Setup
+
+- [x] 1.1 Create a branch `fix/follow-multi-hop-redirects` off `main`, leaving the unrelated uncommitted local changes untouched
+
+## 2. Tests (red)
+
+- [x] 2.1 Add unit test: a two-hop redirect chain (A → B → C) returns 200 and the `.rpm` file is cached
+- [x] 2.2 Add unit test: a redirect loop on the only mirror returns 502, not 3xx
+- [x] 2.3 Add unit test: mirror 1 exceeds the redirect limit and mirror 2 returns 200
+- [x] 2.4 Add unit test: a TLS mirror (`httptest.NewTLSServer`, injected `Transport`) redirecting to `http` returns 502 and the `http` target is never requested
+- [x] 2.5 Confirm the new tests fail (except where current behaviour already passes) and the existing redirect tests still pass
+
+## 3. Implementation
+
+- [x] 3.1 Add the `maxRedirects = 10` constant in `pkg/pkgproxy/proxy.go`
+- [x] 3.2 Replace the single redirect `if` in `tryMirrors` with a loop bounded by `maxRedirects`, logging each hop
+- [x] 3.3 Reject HTTPS → HTTP hops; on hop-limit or downgrade failure, close the response, set `rsp = nil` and `err`, log a warning, and move to the next mirror
+- [x] 3.4 Update the `tryMirrors` doc comment to describe multi-hop following
+
+## 4. Docs
+
+- [x] 4.1 Update `docs/architecture.md`: up to 10 redirects per attempt, HTTPS → HTTP downgrades rejected
+- [x] 4.2 Add `CHANGELOG.md` `[Unreleased]` entries: **Fixed** "Follow multi-hop upstream redirects, restoring package caching for Fedora COPR repositories" and **Changed** "Reject upstream redirects that downgrade from HTTPS to HTTP"
+
+## 5. Verification
+
+- [x] 5.1 Run `make ci-check` and confirm it passes
+- [x] 5.2 Run `make e2e` and confirm all distros pass, including the four `*/COPR` subtests
+- [x] 5.3 Request a Codex review of the diff and address findings

openspec/specs/upstream-redirects/spec.md

@@ -0,0 +1,44 @@
+## Requirements
+
+### Requirement: Follow multi-hop upstream redirects
+The proxy SHALL follow upstream redirect responses (301, 302, 303, 307, 308) for each mirror attempt, up to a maximum of 10 redirect hops. Every hop SHALL replay the original request method and buffered body, with the same filtered request headers.
+
+#### Scenario: Two-hop redirect chain is followed and cached
+- **WHEN** a client requests a cacheable `.rpm` file and the mirror redirects A → B → C, where C returns 200
+- **THEN** the proxy returns 200 with C's body to the client
+- **AND** the file is written to the cache
+
+#### Scenario: Single redirect still works
+- **WHEN** the mirror returns a single redirect to a location that returns 200
+- **THEN** the proxy returns 200 with the redirected body
+
+### Requirement: Redirect hop limit
+When a mirror is still redirecting after 10 hops, the proxy SHALL treat that mirror attempt as failed and continue with the next mirror.
+
+#### Scenario: Redirect loop exceeds the limit
+- **WHEN** the only mirror redirects in a loop that never returns a non-redirect response
+- **THEN** the proxy responds with 502 Bad Gateway
+- **AND** the client does not receive a 3xx response
+
+#### Scenario: Fallback to next mirror after the limit is exceeded
+- **WHEN** mirror 1 exceeds the redirect limit and mirror 2 returns 200
+- **THEN** the proxy returns 200 with mirror 2's body
+
+### Requirement: Reject HTTPS to HTTP redirect downgrades
+The proxy SHALL reject any redirect hop from an `https` URL to an `http` location, including the first hop, and treat that mirror attempt as failed. A redirect from an `http` URL to another `http` location SHALL still be followed.
+
+#### Scenario: HTTPS mirror redirects to plain HTTP
+- **WHEN** the only mirror is served over HTTPS and redirects to an `http` location
+- **THEN** the proxy does not request the `http` location
+- **AND** the proxy responds with 502 Bad Gateway
+
+#### Scenario: HTTP mirror redirects to HTTP
+- **WHEN** a mirror configured with an `http` URL redirects to another `http` location that returns 200
+- **THEN** the proxy returns 200 with the redirected body
+
+### Requirement: Never pass redirects to the client
+The proxy SHALL NOT forward an upstream 3xx redirect response to the client. If no mirror yields a usable response, the proxy SHALL respond with 502 Bad Gateway.
+
+#### Scenario: All mirrors fail due to redirect errors
+- **WHEN** every mirror either exceeds the redirect limit or attempts an HTTPS to HTTP downgrade
+- **THEN** the proxy responds with 502 Bad Gateway

pkg/pkgproxy/proxy.go

@@ -104,6 +104,9 @@ var (
 	// Base delay for exponential backoff between retry attempts (1s, 2s, 4s, ...)
 	retryBaseDelay = 1 * time.Second
 
+	// Maximum number of redirect hops followed per mirror attempt
+	maxRedirects = 10
+
 	// Status codes which will trigger a new request to the "Location" header
 	redirectStatusCodes = []int{
 		301,
@@ -327,9 +330,10 @@ func (pp *pkgProxy) ForwardProxy(next echo.HandlerFunc) echo.HandlerFunc {
 	}
 }
 
-// tryMirrors iterates the mirrors for repo in order, following one redirect per mirror,
-// and returns the first 200 response. Each mirror is attempted up to the configured
-// number of retries (useful when a redirector like download.fedoraproject.org sends
+// tryMirrors iterates the mirrors for repo in order, following up to maxRedirects
+// redirects per mirror attempt (HTTPS to HTTP downgrades are rejected), and returns
+// the first 200 response. Each mirror is attempted up to the configured number of
+// retries (useful when a redirector like download.fedoraproject.org sends
 // traffic to a broken mirror — retrying may yield a different, working mirror).
 // If no mirror returns 200, the last non-nil response (possibly non-200) is returned
 // with a nil error. A non-nil error is only returned when the last mirror attempt
@@ -362,35 +366,47 @@ func (pp *pkgProxy) tryMirrors(ctx context.Context, rid string, req *http.Reques
 			}
 
 			upstreamPath := path.Join(mirror.Path, strings.TrimPrefix(req.URL.Path, "/"+repo))
-			rsp, err = pp.forwardClientRequestToOrigin(ctx, rid, req, &url.URL{
+			target := &url.URL{
 				Scheme:   mirror.Scheme,
 				Host:     mirror.Host,
 				Path:     upstreamPath,
 				RawQuery: req.URL.RawQuery,
-			}, reqBody)
+			}
+			rsp, err = pp.forwardClientRequestToOrigin(ctx, rid, req, target, reqBody)
 			if err != nil {
 				slog.Warn("upstream request failed", "request_id", rid, "mirror_index", i, "attempt", attempt, "error", err)
 				break // connection-level error, skip to next mirror
 			}
 			slog.Info("upstream response", "request_id", rid, "status", rsp.Status, "headers", rsp.Header)
 
-			// Follow HTTP redirects.
-			if utils.Contains(redirectStatusCodes, rsp.StatusCode) {
+			// Follow HTTP redirects up to maxRedirects hops.
+			for hop := 0; utils.Contains(redirectStatusCodes, rsp.StatusCode); hop++ {
 				location, locErr := rsp.Location()
 				_ = rsp.Body.Close()
 				rsp = nil
-				if locErr != nil {
+				switch {
+				case locErr != nil:
 					err = locErr
+				case hop == maxRedirects:
+					err = fmt.Errorf("stopped after %d redirects", maxRedirects)
+				case target.Scheme == "https" && location.Scheme == "http":
+					err = fmt.Errorf("refusing redirect from %s to %s: HTTPS to HTTP downgrade", target.Redacted(), location.Redacted())
+				}
+				if err != nil {
 					slog.Warn("upstream request failed", "request_id", rid, "mirror_index", i, "attempt", attempt, "error", err)
 					break // bad redirect, skip to next mirror
 				}
-				rsp, err = pp.forwardClientRequestToOrigin(ctx, rid, req, location, reqBody)
+				target = location
+				rsp, err = pp.forwardClientRequestToOrigin(ctx, rid, req, target, reqBody)
 				if err != nil {
 					slog.Warn("upstream request failed", "request_id", rid, "mirror_index", i, "attempt", attempt, "error", err)
 					break // connection-level error, skip to next mirror
 				}
 				slog.Info("upstream response", "request_id", rid, "status", rsp.Status, "headers", rsp.Header)
 			}
+			if rsp == nil {
+				break
+			}
 
 			if rsp.StatusCode == http.StatusOK {
 				return rsp, nil