[cleygraf]

Problem

A firewall in front of an upstream registry (Sonatype Firewall Pro, for one) denies an artifact with 403 for npm, PyPI, Maven and OCI, and with 409 for NuGet, and explains the denial in the response body. fetch.Fetcher reports those responses as an error, so every artifact download handler answered with its own generic 502 Bad Gateway. The package manager then reports a proxy fault, and the reason the artifact was refused never reaches the user.

Change

Recover the status code and body from the fetch error and forward them:

• serveUpstreamBlock writes the upstream status and report body (JSON body keeps application/json, anything else is text/plain), and serveArtifactError routes the plain-text download handlers through it, so composer, conan, conda, cran, debian, julia, nuget, pypi, rpm, cargo, pub, go, gem and maven all behave the same way.
• npm keeps its JSON error shape, but a policy denial is passed through as the upstream sent it.
• container answers a refused blob with an OCI 403 DENIED instead of 502 INTERNAL_ERROR.

Upstream 5xx responses and transport errors still become 502.

The fetch layer exposes neither the status nor the body as a typed error, so both are read back out of its message (unexpected status %d: %s); the helper is the one place that knows this format. git-pkgs/registries#129 adds fetch.StatusError, which would let this drop the parsing.

Test

internal/handler/upstream_block_test.go:

• TestUpstreamStatus — wrapped and unwrapped denials, transport errors and fetch sentinels.
• TestServeArtifactErrorForwardsPolicyBlock — 403 and 409 forwarded verbatim with the right content type; upstream 5xx and transport errors still 502.
• TestMavenHandler_ForwardsUpstreamPolicyBlock — a real fetch.Fetcher against an upstream that answers 403 with a report body; the client receives that status and body unchanged.
• TestNPMHandler_ForwardsUpstreamPolicyBlock — same for the npm tarball path.
• TestContainerHandler_BlobPolicyBlockIsDenied — a refused blob is reported as OCI DENIED.

go build ./... && go vet ./... && go test ./... -count=1 pass.

Context

Found while running the proxy in front of Sonatype Firewall Pro, where a blocked package was indistinguishable from a broken proxy. The change is generic: any upstream that refuses an artifact with 403/409 and a body benefits.

Companion change: git-pkgs/registries#129 — a 4xx answer no longer counts against the per-host circuit breaker, so a run of policy denials cannot open it for every registry that host serves.

Summary by CodeRabbit

• Bug Fixes
  • Upstream policy denials with HTTP 403 or 409 are now returned to clients with the upstream status and response body, rather than a generic 502 error.
  • Responses preserve appropriate content types for JSON and plain-text bodies.
  • Container image requests blocked with HTTP 403 now receive an OCI DENIED response that includes the upstream reason when available.
  • Other upstream failures continue to return a generic 502 response.

[andrew]

Thanks, forwarding the upstream's 403/409 instead of a generic 502 is the right behaviour. A few things before this can go in:

1. upstreamStatus parses the status and body back out of the fetcher's error string with a regex. registries is ours to change, and git-pkgs/registries#129 already adds fetch.StatusError{Code, Body}. Once that is merged and tagged, please bump registries here and replace upstreamStatus and upstreamStatusPattern with errors.As(err, &statusErr). TestUpstreamStatus can then go, or shrink to an errors.As check.

2. The fetcher truncates error bodies to 1024 bytes (maxErrBodySize), but serveUpstreamBlock labels any body starting with { as application/json. A JSON report over 1KB gets served cut off mid-object as invalid JSON, which npm then fails to parse. Either only use the JSON content type when json.Valid(body) passes, or carry the upstream Content-Type on StatusError in registries#129 and forward that.

3. In container.go a refused blob answers DENIED with a fixed "blob refused by upstream policy" message and the Warn log drops the body too, so Docker clients still lose the reason the upstream gave. Put the upstream body in the OCI error message (or detail) and include it in the log line.

4. The comments repeat the commit message. Cut the doc comment on serveUpstreamBlock to one line, drop the upstreamStatusPattern comment along with the regex, and remove the comments added in npm.go and above the 403 check in container.go.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Artifact handlers now forward selected upstream policy-denial responses. The container blob handler maps upstream 403 errors to OCI DENIED responses. The change also updates two Go dependencies.

Changes: Upstream denial responses

Layer / File(s)	Summary
Shared upstream response forwarding internal/handler/upstream_block.go, internal/handler/handler.go, internal/handler/npm.go, internal/handler/upstream_block_test.go, go.mod	The shared handler forwards upstream 403 and 409 status codes and response bodies. It selects a content type based on the upstream type and whether the body is valid JSON. Maven and npm error paths use this handling. Tests cover status, body, content type, and fallback behavior. go.mod updates github.com/git-pkgs/registries and go.yaml.in/yaml/v3.
Container blob denial response internal/handler/container.go, internal/handler/upstream_block_test.go	For upstream 403 blob-fetch errors, the container handler returns HTTP 403 with an OCI DENIED response. It includes a nonblank upstream response body in the message.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ArtifactClient
  participant ArtifactHandler
  participant Upstream
  ArtifactClient->>ArtifactHandler: Request artifact
  ArtifactHandler->>Upstream: Fetch artifact
  Upstream-->>ArtifactHandler: Return policy-denial status and body
  ArtifactHandler-->>ArtifactClient: Forward eligible status and response

Loading

Suggested labels: bug

Suggested reviewers: andrew

Merge Risk: Minimal · up to 84902

Eligible upstream denials are returned in the intended response formats, while other failures retain their fallback behavior. No issue requiring a fix before merge remains established.

Security architecture risk: Low · up to 84902

Denial details become visible to clients, but the reviewed paths retain local policy checks, failure isolation, and browser sandboxing. No introduced security failure was established. Incomplete coverage and unverified deployment assumptions prevent a minimal-risk assessment.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The new exposure is upstream-controlled denial content returned to clients of existing artifact routes and included in denial logs. Its body size is bounded by the production fetch producer; the reviewed change does not introduce a new public route or grant additional upstream authority.

Security Findings and Attack Paths

• inferred — The relevant browser attack path is an upstream-controlled denial body rendered on the proxy origin. MIME selection alone does not prevent active content, but the existing route-wide sandbox disables scripts and assigns an opaque origin. No bypass was established in the inspected application routes; preservation of these headers by external ingress remains unverified.

Trust Boundaries and Controls

• observed — The forwarding helper copies the selected status, content type, and body rather than arbitrary upstream headers. Existing local policy outcomes remain authoritative, and held requests retain their 503 response with Retry-After before denial forwarding is considered.

Resilience and Maintainability Implications

• observed — The inspected failure transitions preserve artifact visibility boundaries: denial errors do not produce stored artifacts, shared fetch completion removes in-flight state and wakes waiters, and failed leases permit later recovery. Cancellation and concurrent waiters do not convert a denial into a successful artifact result.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "Forward upstream policy denials instead of masking them as 502" matches the main change. The raw summary shows upstream 403 and 409 responses are now forwarded in artifact, npm, and contain…
Docstring Coverage		Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "Forward upstream policy denials instead of masking them as 502" matches the main change. The raw summary shows upstream 403 and 409 responses are now forwarded in artifact, npm, and container handlers instead of being replaced by a generic 502 Bad Gateway. The objectives confirm that 5xx and transport errors still return 502. The title is short, single-sentence, and specific, so it passes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 5 files. (1 skipped: 1 unsupported.)

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 20df48b0-1159-4196-bfcf-51a814b76992

Commits

Reviewing files that changed from the base of the PR and between 72aa8fd and 84902fb.

Files ignored due to path filters: 1

• go.sum is excluded by !**/*.sum

Files selected for processing: 6

• go.mod
• internal/handler/container.go
• internal/handler/handler.go
• internal/handler/npm.go
• internal/handler/upstream_block.go
• internal/handler/upstream_block_test.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[cleygraf]

Rebased onto current main and reworked per your review:

• registries is at v0.13.2; upstreamStatus/upstreamStatusPattern and the regex are gone in favour of errors.As(err, &statusErr). TestUpstreamStatus is dropped — the handler table now covers the wrapped error.
• serveUpstreamBlock forwards StatusError.ContentType. A body that the 1 KB truncation left invalid is labelled text/plain instead of being served as broken JSON.
• container.go: the OCI DENIED message and the warn log now carry the upstream body, so the client and the log keep the reason the upstream gave.
• Doc comments cut to one line (or removed) as requested in serveUpstreamBlock, npm.go and container.go.

go build ./..., go vet ./internal/handler/ and go test ./... pass locally. The rebase also resolved go.mod/go.sum against the sbom bump on main.

[andrew]

Thanks, all four points from the last round are addressed. Two things left:

1. Go module zips still answer 502. internal/handler/go.go fetches the zip with GetOrFetchArtifactFromURL but handles the error itself instead of going through serveArtifactError, so a 403 from the upstream reaches go as "failed to fetch module". Every other artifact download now goes through the new helper. Add h.proxy.serveUpstreamBlock(w, err) after the writeUpstreamHeld check there, with a test that has the upstream answer the zip request with a 403.

2. A few comments restate the name of what they sit on. Remove the doc comment on jsonMediaType, cut the one on upstreamBlockContentType to a single line or drop it, and remove the comments above TestMavenHandler_ForwardsUpstreamPolicyBlock, TestNPMHandler_ForwardsUpstreamPolicyBlock and TestContainerHandler_BlobPolicyBlockIsDenied.