619842 - Link to SVN history of page in mozilla.org footer is broken

Status: VERIFIED FIXED

(www.mozilla.org :: General, defect)

Description

[David Boswell]

The link to Page History in the footer of mozilla.org pages is broken.  For instance, from the Mozilla-based Applications page, the link points to:

http://viewvc.svn.mozilla.org/vc/projects/mozilla.org/trunk/projects%2Fmozilla-based.html?view=log

and brings up this error:

Not Found

The requested URL /vc/projects/mozilla.org/trunk/projects/mozilla-based.html was not found on this server.

Is this a regression from the recent r79421 checkin that touched the SvnHistoryLink.php file?

Comment 2

[Chris More [:cmore]]

David:

Yeah, it looks like the problem was introduced in http://viewvc.svn.mozilla.org/vc?view=rev&revision=79421 by the urlencode() of $filename variable.

The problem is that the $filename variable is not a single URL substring, but the absolute path to the HTML file from the root of mozilla.org. For example, at this URL: https://www.mozilla.org/about/forums/ the value of the $filename variable is /about/forums/index.html. I think the past revision assumed that $filename was a filename and not a path.

A few ways to fix this...

1) Change line 34 to:

$uri = self::VIEWVC . str_replace("%2F", "/", urlencode($filename)) . "?view=log"; 

2) Adjust Apache to allow encoded slashes: http://httpd.apache.org/docs/2.2/mod/core.html#allowencodedslashes

3) Remove the urlencode(), but probably not the best solution for XSS reasons.

-Chris

(In reply to comment #0)
> 
> Is this a regression from the recent r79421 checkin that touched the
> SvnHistoryLink.php file?

Comment 3

[David Boswell]

Chris, thanks for options in comment #2.  I'm not the right person to say which of those are the best option, so let's see what Reed or Gordon says.

Comment 4

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #0) 
> Is this a regression from the recent r79421 checkin that touched the
> SvnHistoryLink.php file?

Yes it is. I don't think urlencode() is the best solution here. It probably would be better to use htmlspecialchars() or (my personal favorite) htmlentities() instead.

Comment 5

[Chris More [:cmore]]

(In reply to comment #4)
> 
> Yes it is. I don't think urlencode() is the best solution here. It probably
> would be better to use htmlspecialchars() or (my personal favorite)
> htmlentities() instead.

I would agree. It is not like these URL is being passed through as a querystring thus urlencode() is really just doing its job. 

htmlentities() or htmlspecialchars() would be a good choice.

Comment 6

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Use htmlentities() instead of urlencode()

I don't have a full checkout of the directory structure, but here's a patch that should fix the issue. (You'll want to stage it first to be sure.) Just watch that the filenames in the patch don't match. (I used 'old' and 'new'.)

Comment 7

[Reed Loden [:reed]]

Comment on attachment 503891 [details] [diff] [review]
Use htmlentities() instead of urlencode()

I'd almost rather you keep the current code and just change urlencode() to htmlentities(). I'm worried that by doing htmlentities(), part of the query string will be modified incorrectly. Can you set up a local test instance to see what happens?

Comment 8

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #7)
> Comment on attachment 503891 [details] [diff] [review]
> Use htmlentities() instead of urlencode()
> 
> I'd almost rather you keep the current code and just change urlencode() to
> htmlentities(). I'm worried that by doing htmlentities(), part of the query
> string will be modified incorrectly.
Well, that's not likely to happen, since (1) htmlentities() is designed for exactly this purpose of outputting into HTML files, and (2) we pretty much know everything that will be coming through into $uri.

> Can you set up a local test instance to
> see what happens?
I will eventually have to do that, yeah, but I'll need your help. Last time I tried, things got hairy. :)

Comment 9

[Chris More [:cmore]]

+1 Gordon. These are just nodes within the SVN tree structure and not some arbitrary block of HTML code. Changing urlencode() to htmlentities() is the easiest change and a local test traversing the SVN would confirm.

Comment 10

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #7)
> Comment on attachment 503891 [details] [diff] [review]
> Use htmlentities() instead of urlencode()
> 
> I'd almost rather you keep the current code and just change urlencode() to
> htmlentities(). I'm worried that by doing htmlentities(), part of the query
> string will be modified incorrectly. Can you set up a local test instance to
> see what happens?

Committed to stage in r82433. Looks like it works fine.

I'll need someone (reed?) to confirm and merge to trunk.

Comment 12

[Gervase Markham [:gerv]]

It's four months later and this is still broken. Who is responsible for checking this patch in on trunk? Can I do it?

Gerv

Comment 13

[Milos Dinic [:Milos]]

The link on mozilla.org prod is working fine for me. Shouldn't this be resolved/fixed?

Comment 14

[Gervase Markham [:gerv]]

It is only broken for subdirectories.

So on:
http://www.mozilla.org/about/forums/
the link is to:
http://viewvc.svn.mozilla.org/vc/projects/mozilla.org/trunk/about%2Fforums%2Findex.html?view=log
when it should be to:
http://viewvc.svn.mozilla.org/vc/projects/mozilla.org/trunk/about/forums/index.html?view=log

If it's working for you, please give details of which pages it is working on.

Gerv

Comment 15

[Milos Dinic [:Milos]]

Sorry, I didn't actually click the links, just took a look at status bar. It works for index.html in the root only.

Comment 16

[David Boswell]

(In reply to comment #12)
> It's four months later and this is still broken. Who is responsible for
> checking this patch in on trunk? Can I do it?

Gerv, sure, that would be great.  The review and check-in process for technical bugs on www.mozilla.org has been pretty broken for a while so any help here would be great.

Comment 17

[Gervase Markham [:gerv]]

Sending        includes/MozillaOrg/SvnHistoryLink.php
Transmitting file data .
Committed revision 91075.

Gerv

Comment 18

[David Boswell]

Gerv, thanks for checking in.  It seems to be working fine for me, but I'll let Gordon verify to make sure everything is working as he intended.

Comment 19

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #18)
> Gerv, thanks for checking in.  It seems to be working fine for me, but I'll
> let Gordon verify to make sure everything is working as he intended.

Code looks good, though the commit message has my name wrong. ;)

Gerv, could you also take care of bug 521706?

Marking this one as VERIFIED.

Comment 20

[Gervase Markham [:gerv]]

Sorry about misspelling your name!

Gerv