consistent clean_path_info separator by akostadinov · Pull Request #2257 · rack/rack

akostadinov · 2024-10-30T14:18:20Z

It is inconsistent to prepend / while earlier clean.join(::File::SEPARATOR).

Additionally request PATH_INFO should always start with a slash so we
can remove the conditional prepending and just always prepend.

This can also prevent path traversal attacks in case some code checks
the request path beginning using string comparison or regular expression
but the underlying server implementation allowed a request path without
a leading slash through.

I can say that at least puma fails fast on request paths without a
leading slash but I see no harm in being extra sure.

P.S. actually I'd be even happier to make it join("/") instead of clean.join(::File::SEPARATOR). Is there any arch that would need a different separator than /?

It is inconsistent to prepend `/` while earlier `clean.join(::File::SEPARATOR)`. Additionally request `PATH_INFO` should always start with a slash so we can remove the conditional prepending and just always prepend. This can also prevent path traversal attacks in case some code checks the request path beginning using string comparison or regular expression but the underlying server implementation allowed a request path without a leading slash through. I can say that at least puma fails fast on request paths without a leading slash but I see no harm in being extra sure.

jeremyevans · 2024-10-30T15:52:38Z

-      clean_path = clean.join(::File::SEPARATOR)
-      clean_path.prepend("/") if parts.empty? || parts.first.empty?
-      clean_path
+      "#{::File::SEPARATOR}#{clean.join(::File::SEPARATOR)}"


This results in different behavior for relative paths (e.g. "a", "b/a"). Previously, clean_path_info did not prepend a slash, and now it would. For that reason, I doubt it would be acceptable.

Looking at the history, it appears to be called with PATH_INFO, which generally should be empty or start with a slash. However, the specs have tests with relative paths. Those tests happen to work with this, but that seems like an accident and not deliberate. For example, there is a test for "test/..", but not a test for "test" by itself.

As we cannot be sure how users are using clean_path_info, I don't think we should accept the behavior change.

Could you say in what case PATH_INFO would be empty or relative (I couldn't reproduce this in practice with puma)? What valid HTTP request will make it empty?

I think it is important for this method to be as safe as possible for the intended purpose - to make the PATH_INFO predictable to compare or use regular expression on. Rather than some unintended use cases.

Presently just figuring out what would be the actual path of a request is rather hard. And it should not be hard to do it right.

This means, as I mentioned to prevent https://owasp.org/www-community/attacks/Path_Traversal

HTTP 1.1 supports only (https://datatracker.ietf.org/doc/html/rfc9112#name-request-target)

absolute path

full URL (in which case server sets PATH_INFO to an absolute path)

host:port (in CONNECT requests but we are not a proxy)

* (single star for server global OPTIONS request, but we can handle this as an individual check)

So basically I see no valid use cases where the result should be a relative path.

Just to clarify, we do check those four scenarios here:

rack/lib/rack/lint.rb

Lines 364 to 389 in fc541f3

## * The <tt>PATH_INFO</tt>, if provided, must be a valid request target or an empty string.

if env.include?(PATH_INFO)

case env[PATH_INFO]

when REQUEST_PATH_ASTERISK_FORM

## * Only <tt>OPTIONS</tt> requests may have <tt>PATH_INFO</tt> set to <tt>*</tt> (asterisk-form).

unless env[REQUEST_METHOD] == OPTIONS

raise LintError, "Only OPTIONS requests may have PATH_INFO set to '*' (asterisk-form)"

end

when REQUEST_PATH_AUTHORITY_FORM

## * Only <tt>CONNECT</tt> requests may have <tt>PATH_INFO</tt> set to an authority (authority-form). Note that in HTTP/2+, the authority-form is not a valid request target.

unless env[REQUEST_METHOD] == CONNECT

raise LintError, "Only CONNECT requests may have PATH_INFO set to an authority (authority-form)"

end

when REQUEST_PATH_ABSOLUTE_FORM

## * <tt>CONNECT</tt> and <tt>OPTIONS</tt> requests must not have <tt>PATH_INFO</tt> set to a URI (absolute-form).

if env[REQUEST_METHOD] == CONNECT || env[REQUEST_METHOD] == OPTIONS

raise LintError, "CONNECT and OPTIONS requests must not have PATH_INFO set to a URI (absolute-form)"

end

when REQUEST_PATH_ORIGIN_FORM

## * Otherwise, <tt>PATH_INFO</tt> must start with a <tt>/</tt> and must not include a fragment part starting with '#' (origin-form).

when ""

# Empty string is okay.

else

raise LintError, "PATH_INFO must start with a '/' and must not include a fragment part starting with '#' (origin-form)"

end

end

@ioquatix , I wonder why empty string is accepted. RFC clearly reads:

If the target URI's path component is empty, the client MUST send "/" as the path within the origin-form of request-target.

Additionally the RFC states:

Recipients of an invalid request-line SHOULD respond with either a 400 (Bad Request) error or a 301 (Moved Permanently) redirect with the request-target properly encoded. A recipient SHOULD NOT attempt to autocorrect and then process the request without a redirect, since the invalid request-line might be deliberately crafted to bypass security filters along the request chain.

Which is to a big extend my concern.

ioquatix · 2024-10-30T20:54:42Z

We can check the reason why this method exists: 9a74ba3

The point is to convert a PATH_INFO to a file path.

The usage is something like this:

@path = F.join(@root, clean_path_info)

There are some issues IMHO:

SEPS = Regexp.union(*[::File::SEPARATOR, ::File::ALT_SEPARATOR].compact) seems extremely odd for PATH_INFO as it would definitely follow the URL specification. I'd be concerned that PATH_INFO interpreted with ALT_SEPARATOR can be used for cache poisoning attacks.
It was originally ::File.join(*clean) and was changed here to Array#join due to performance: 15ba6c7 but in all cases had logic to the effect of clean.unshift '/' if parts.empty? || parts.first.empty?.

I find it odd that it returns what effectively looks like an absolute path, yet it's treated as a relative path. I think this is error prone, e.g.

File.join("/srv/http/mysite/public", "/background.png")
# => "/srv/http/mysite/public/background.png"

File.expand_path("/background.png", "/srv/http/mysite/public")
# => "/background.png"

In other words, it looks easy to make a mistake and get the wrong behaviour, IOW, I think the way the method works is inherently unsafe.

I think the safe solution is to pass in the full root, e.g.

Rack::Utils.full_path(root, env["PATH_INFO"]) -> file path

For security reasons, we should assert that the result still has root as a prefix. I don't have a strong opinion about the naming conventions. And I could be convinced that the current behaviour is okay.

To clarify, I think there are some things we should do:

Add better documentation to this method about how it should be used.
Perhaps audit existing usage in public apps (GitHub code search can help) to see if there are issues in the wild.
Consider adding a new method to replace the old one.
Deprecate the old method if it's deemed a security issue.

ioquatix · 2025-07-31T01:12:20Z

I'm going to close this PR in favour of maybe doing something in #2302

jeremyevans reviewed Oct 30, 2024

View reviewed changes

ioquatix force-pushed the main branch from c9a26a1 to b0c39b7 Compare March 4, 2025 05:28

ioquatix mentioned this pull request Mar 10, 2025

Don't use file separator for splitting path info. #2302

Open

ioquatix force-pushed the main branch from d5e4576 to fac9a7a Compare July 31, 2025 01:07

ioquatix closed this Jul 31, 2025

Navigation Menu

consistent clean_path_info separator - #2257

consistent clean_path_info separator#2257
akostadinov wants to merge 1 commit into
rack:mainfrom
akostadinov:main

akostadinov commented Oct 30, 2024 •

edited

Loading

Uh oh!

jeremyevans Oct 30, 2024

Uh oh!

akostadinov Oct 30, 2024 •

edited

Loading

Uh oh!

akostadinov Oct 30, 2024

Uh oh!

ioquatix Oct 30, 2024

Uh oh!

akostadinov Oct 30, 2024

Uh oh!

akostadinov Oct 30, 2024

Uh oh!

ioquatix commented Oct 30, 2024 •

edited

Loading

Uh oh!

ioquatix commented Jul 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	## * The <tt>PATH_INFO</tt>, if provided, must be a valid request target or an empty string.
	if env.include?(PATH_INFO)
	case env[PATH_INFO]
	when REQUEST_PATH_ASTERISK_FORM
	## * Only <tt>OPTIONS</tt> requests may have <tt>PATH_INFO</tt> set to <tt>*</tt> (asterisk-form).
	unless env[REQUEST_METHOD] == OPTIONS
	raise LintError, "Only OPTIONS requests may have PATH_INFO set to '*' (asterisk-form)"
	end
	when REQUEST_PATH_AUTHORITY_FORM
	## * Only <tt>CONNECT</tt> requests may have <tt>PATH_INFO</tt> set to an authority (authority-form). Note that in HTTP/2+, the authority-form is not a valid request target.
	unless env[REQUEST_METHOD] == CONNECT
	raise LintError, "Only CONNECT requests may have PATH_INFO set to an authority (authority-form)"
	end
	when REQUEST_PATH_ABSOLUTE_FORM
	## * <tt>CONNECT</tt> and <tt>OPTIONS</tt> requests must not have <tt>PATH_INFO</tt> set to a URI (absolute-form).
	if env[REQUEST_METHOD] == CONNECT \|\| env[REQUEST_METHOD] == OPTIONS
	raise LintError, "CONNECT and OPTIONS requests must not have PATH_INFO set to a URI (absolute-form)"
	end
	when REQUEST_PATH_ORIGIN_FORM
	## * Otherwise, <tt>PATH_INFO</tt> must start with a <tt>/</tt> and must not include a fragment part starting with '#' (origin-form).
	when ""
	# Empty string is okay.
	else
	raise LintError, "PATH_INFO must start with a '/' and must not include a fragment part starting with '#' (origin-form)"
	end
	end

Conversation

akostadinov commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremyevans Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

akostadinov Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

ioquatix Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

akostadinov Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

akostadinov Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

ioquatix commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ioquatix commented Jul 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akostadinov commented Oct 30, 2024 •

edited

Loading

akostadinov Oct 30, 2024 •

edited

Loading

ioquatix commented Oct 30, 2024 •

edited

Loading