Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix raw path handling in strip prefix #2382

Merged

Conversation

m3co-code
Copy link
Contributor

This PR fixes forwarding of URL encoded characters to the backend servers.

Given that Traefik receives a request path in the form: /some/a%2Fb it should forward the same path to the backend. This is the case when using e.g. the rule type PathPrefix, but as soon as the rule type is either PathPrefixStrip or PathPrefixStripRegex the path was forwarded in the decoded form like: /some/a/b.

The reason why this solution fixes our problem is that go uses the String() method of the URL object when it constructs the request. The docs state for this:

// String reassembles the URL into a valid URL string.
// The general form of the result is one of:
//
// scheme:opaque?query#fragment
// scheme://userinfo@host/path?query#fragment
//
// If u.Opaque is non-empty, String uses the first form;
// otherwise it uses the second form.
// To obtain the path, String uses u.EscapedPath().

For us the relevant part is with u.EscapedPath() and there the docs state:

// EscapedPath returns the escaped form of u.Path.
// In general there are multiple possible escaped forms of any path.
// EscapedPath returns u.RawPath when it is a valid escaping of u.Path.
// Otherwise EscapedPath ignores u.RawPath and computes an escaped
// form on its own.
// The String and RequestURI methods use EscapedPath to construct
// their results.
// In general, code should call EscapedPath instead of
// reading u.RawPath directly.

To understand the problem, note the sentence: EscapedPath returns u.RawPath when it is a valid escaping of u.Path. By stripping only the Path and not the RawPath we violated this statement and therefore the http package decided to use the Path.

@timoreimann
Copy link
Contributor

We ran into the problem this PR fixes in our internal infrastructure and validated that the code change fixes things.

Is that good enough to classify the PR as kind/bug/fix, or is there more needed?

@ldez
Copy link
Member

ldez commented Nov 9, 2017

Seems related to containous/oxy#30

@m3co-code
Copy link
Contributor Author

@ldez those issues are not related.

@ldez ldez added this to the 1.5 milestone Nov 18, 2017
@ldez ldez force-pushed the fix-raw-path-handling-in-strip-prefix branch from 462d00d to b7c31e8 Compare November 18, 2017 22:05
@ldez ldez changed the base branch from master to v1.4 November 18, 2017 22:06
@ldez ldez modified the milestones: 1.5, 1.4 Nov 18, 2017
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM 👏

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM

@kachkaev
Copy link
Contributor

@marco-jantke do you think that fixing #1957 could be somewhat similar? That issue has been around for a while.

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

Successfully merging this pull request may close these issues.

None yet

7 participants