Skip to content

fix: preserve percent-encoded path when forwarding#192

Merged
SasSwart merged 1 commit intocoder:mainfrom
ptenkale123:ptenkale/fix-preserve-encoded-path
Apr 24, 2026
Merged

fix: preserve percent-encoded path when forwarding#192
SasSwart merged 1 commit intocoder:mainfrom
ptenkale123:ptenkale/fix-preserve-encoded-path

Conversation

@ptenkale123
Copy link
Copy Markdown

@ptenkale123 ptenkale123 commented Apr 22, 2026

Summary

The HTTP/HTTPS forwarder built the upstream URL from req.URL.Path alone, which is the decoded path. url.URL.String() then re-encoded it and emitted any %2F as a literal /. Upstreams that treat /@scope%2Fname and /@scope/name as distinct paths — notably AWS CodeArtifact's scoped-package API — return 400 before reaching auth.

error: GET <url>/npm/npm-all/@sentry%2Fbun - 400

Per RFC 3986 §2.2, proxies shouldn't unconditionally decode reserved characters.

Fix

Copy req.URL.RawPath into the forwarded url.URL alongside Path. Go's url.URL.EscapedPath() (called by String()) returns RawPath verbatim when it's a valid encoding of Path, so the original %2F survives on the wire.

Test plan

  • New TestForwardPreservesPercentEncodedPath in proxy/proxy_encoding_test.go — sends /npm/npm-all/@sentry%2Fbun through the proxy to a local httptest backend and asserts the backend's r.RequestURI still contains %2F.
  • Verified the test fails without the fix (backend sees /@sentry/bun) and passes with it.
  • Full go test ./proxy/... suite green.

@mtojek mtojek requested a review from SasSwart April 23, 2026 07:51
Copy link
Copy Markdown
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick look, but will defer the final review to @SasSwart.

Comment thread proxy/proxy.go
Comment thread proxy/proxy_encoding_test.go Outdated
)

backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
mu.Lock()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm... do we really need the mutex?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah creating a new test file was overkill, just added it to existing one

Comment thread proxy/proxy_encoding_test.go Outdated
got := receivedRawURI
mu.Unlock()

assert.Equal(t, encodedPath, got,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at proxy_test.go and this could be the right moment to bring another helper function. Something like:

	t.Run("PercentEncodedSlash", func(t *testing.T) {
		pt.ExpectRawURI(
			"http://localhost:8080/npm/npm-all/@sentry%2Fbun",
			"jsonplaceholder.typicode.com",
			"/npm/npm-all/@sentry%2Fbun",
		)
	})

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in latest commit

@ptenkale123 ptenkale123 force-pushed the ptenkale/fix-preserve-encoded-path branch 2 times, most recently from fcd06bf to 26f2861 Compare April 23, 2026 22:02
@ptenkale123 ptenkale123 force-pushed the ptenkale/fix-preserve-encoded-path branch from 26f2861 to c540b35 Compare April 23, 2026 22:08
@SasSwart
Copy link
Copy Markdown
Contributor

Review request acknowledged

Copy link
Copy Markdown
Contributor

@SasSwart SasSwart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks!

Comment thread proxy/proxy.go
@SasSwart SasSwart merged commit 6e1bec4 into coder:main Apr 24, 2026
2 checks passed
@ptenkale123
Copy link
Copy Markdown
Author

@SasSwart @mtojek Can we patch this fix in an official release? Just trying gauge how soon we can see this in an official release

@SasSwart
Copy link
Copy Markdown
Contributor

Hey @ptenkale123,

I'm checking for objections internally. If there are none, I will create a new release tomorrow.

@SasSwart
Copy link
Copy Markdown
Contributor

@ptenkale123 this has been released in v0.9.0. Thanks for the contribution. Feel free to test it out and be sure to let us know if there is anything else!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants