Closed Bug 619842 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: davidwboswell, Assigned: GPHemsley)

References

Details

Attachments

(1 file)

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?
Assignee: nobody → reed
OS: Mac OS X → All
Hardware: x86 → All
Duplicate of this bug: 620859
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?
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.
(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.
Depends on: 619263
(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.
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'.)
Assignee: reed → gphemsley
Status: NEW → ASSIGNED
Attachment #503891 - Flags: review?(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?
Attachment #503891 - Flags: review?(reed)
Attachment #503891 - Flags: review?(reed)
(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. :)
+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.
(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.
Assignee: gphemsley → reed
Assignee: reed → gphemsley
Blocks: 521706
Duplicate of this bug: 650652
Blocks: 650652
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
The link on mozilla.org prod is working fine for me. Shouldn't this be resolved/fixed?
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
Sorry, I didn't actually click the links, just took a look at status bar. It works for index.html in the root only.
(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.
Sending        includes/MozillaOrg/SvnHistoryLink.php
Transmitting file data .
Committed revision 91075.

Gerv
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
(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.
Status: RESOLVED → VERIFIED
Sorry about misspelling your name!

Gerv
Component: www.mozilla.org → General
Product: Websites → www.mozilla.org
You need to log in before you can comment on or make changes to this bug.