Opened 8 years ago

Closed 4 years ago

#1570 defect closed wontfix (wontfix)

static.File shouldn't allow excessive /s.

Reported by: jknight Owned by:
Priority: high Milestone: Web2-Gold-Master
Component: web2 Keywords:
Cc: wsanchez, jknight Branch:
Author: Launchpad Bug:

Description

Using static.File('/'), I can successfully get the contents of /etc/resolv.conf with the url http://localhost:8080/etc//////resolv.conf////. There's two things wrong with this:

  1. Empty segments. Either that should be an error or a redirect.
  2. a / after a file (not a directory) should surely be an error.

Change History (11)

comment:1 Changed 8 years ago by wsanchez

  • Cc wsanchez added

comment:2 follow-up: Changed 8 years ago by glyph

I think that is tricky behavior to get right. For example, http://apache.org/foundation////////contributing.html

A slash after a file should definitely be an error though.

comment:3 follow-up: Changed 8 years ago by wsanchez

  • Priority changed from normal to highest

Given the security implications here, this should be a showstopper for a 1.0 web2 release.

comment:4 in reply to: ↑ 2 Changed 8 years ago by jknight

  • Cc jknight added
  • Milestone set to Web2-Gold-Master
  • Priority changed from highest to high

Replying to glyph:

I think that is tricky behavior to get right. For example, http://apache.org/foundation////////contributing.html

That behavior falls under #1: it is incorrect and should not be supported.

comment:5 Changed 7 years ago by rikyu

Wouldn't http://example.com/index.html/ be equivalent to http://example.com/index.html with a PATH_INFO of '/'? This is how apache handles it, although whether that means anything is left as an exercise to the reader.

I've used this approach often as a method of creating semi-clean URLs on webhosts that don't support mod_rewrite, as it's accessible from raw CGI, PHP, even client-side JavaScript. These may not be major concerns for web2, but if it gets us that closer to a web2 release, I think the trailing slash (and any subsequent path after the filename) should just be added to PATH_INFO.

comment:6 follow-up: Changed 7 years ago by exarkun

PATH_INFO is a CGI thing. Environment variables probably aren't the answer here. A slash after a file should definitely be an error. I'm not sure why there's a question about two slashes in a row. What's the motivation to translate /foobar into anything other than root.locateChild('foo').locateChild('').locateChild('bar')? The goal of the fundamental abstraction should be simplicity and consistency. Given that, convenience can be built on top to make the nice thing simple, without compromising the system overall.

comment:7 Changed 7 years ago by rikyu

Yeah, my mistake. I think I was in too much of a hurry to contribute in some way. ;-)

comment:8 in reply to: ↑ 6 Changed 7 years ago by glyph

Replying to exarkun:

I'm not sure why there's a question about two slashes in a row.

Apache does something weird there, and maybe it is worth doing the same weird thing for the sake of compatibility. I'm not dead set on that, but compatibility is the reason I brought it up.

What's the motivation to translate /foobar into anything other than root.locateChild('foo').locateChild().locateChild('bar')?

Ignoring the obviously incorrect format of those API calls ;), I would never propose to treat this differently or specially in the "kernel" of the webserver. If a compatibility hack is desirable, it's desirable specifically in static.File, for compatibility with other webservers' file-serving semantics.

As long as this is a hot topic now, though, why is this so important that it's included in the gold master milestone? It seems to be a small detail, which is arguably not a bug, with no security or performance implications, and few-to-no long-term support issues.

comment:9 in reply to: ↑ 3 Changed 7 years ago by glyph

Replying to wsanchez:

Given the security implications here, this should be a showstopper for a 1.0 web2 release.

Oops. Since you said "there are security implications" and then I said "there aren't" let me clarify:

The example given in the ticket's description involves a static.File at "/", so while the paths look scary, they aren't actually. The problem being discussed isn't that you can access /etc/resolv.conf from an arbitrary file resource; it's that you can access /path/x/y/z with /xy/z from a static file resource serving /path.

comment:10 Changed 4 years ago by exarkun

  • Resolution set to wontfix
  • Status changed from new to closed

Not going to fix this (See #4821). Since twisted.web.static.File does not have the same problem, closing this.

comment:11 Changed 4 years ago by <automation>

  • Owner jknight deleted
Note: See TracTickets for help on using tickets.