#147 open

link#absolute_href is broken

Reported by dstar | January 26th, 2009 @ 02:25 PM | in 0.4.2

There is a bug in link#absolute_href:

54 def absolute_href

 55       if href =~ /^\?/
 56         "#{@session.current_url}#{href}"
 57       elsif href !~ %r{^https?://www.example.com(/.*)} && (href !~ /^\//)
 58         "#{@session.current_url}/#{href}"
 59       else
 60         href
 61       end
 62     end

I'm not positive, but I think Line 55 should be:

if href =~ /^\/?/

unless it's meant to be checking for urls that begin with '?'? If so, it would turn ?foo into 'http://foo.bar.baz?foo', which I'm not sure is legal.

Line 57 should not check for the existence of www.example.com, as that causes it to fail on absolute urls. Assuming that I'm right about line 55, it should also not check that href is a relative url, as it can never be reached if it is: elsif href !~ %r{^https?://[^/]+(/.*)}

Comments and changes to this ticket

  • Bryan Helmkamp

    Bryan Helmkamp January 26th, 2009 @ 02:29 PM

    • Milestone set to 0.4.1
    • State changed from “new” to “open”
    • Assigned user set to “Bryan Helmkamp”
  • dstar

    dstar January 26th, 2009 @ 02:35 PM

    • Assigned user cleared.

    Just checked the RFC -- an HTTP url has the following format:


    So the / after the host/port is required, and 'http:foo.bar.baz?quux' is an illegal url.

  • dstar

    dstar January 26th, 2009 @ 04:15 PM

    I sent you a pull request for the master branch of dstar/webrat on github. I moved absolute_href to Element, since all three things that use it are descended from element, fixed the bug, and added some comments to make it clearer what each branch of the if statement is for.

    I think absolute_href might not be the best name for the method now that it's been moved away from Link, but I couldn't think of a better one that I was sure wouldn't conflict with anything.

  • dstar

    dstar January 28th, 2009 @ 01:10 PM

    Done in branch canonicalize_all_urls of git://github.com/dstar/webrat.git with all specs passing.

  • dstar

    dstar January 28th, 2009 @ 01:13 PM

    Whoops, wrong ticket. Instead of moving absolute_href to Element, that functionality was moved into Session; all urls will be canonicalized before being requested. That fixes both this problem and problems related to detecting internal redirects when redirecting to an absolute href which does not include the domain from a request which did include the domain.

  • Bryan Helmkamp

    Bryan Helmkamp February 8th, 2009 @ 11:23 PM

    • Milestone changed from 0.4.1 to 0.4.2
  • gaffo

    gaffo June 3rd, 2009 @ 10:14 AM

    • Tag set to stale

    It's been quite a while, is this still an issue or should I close this ticket?

  • dstar

    dstar June 5th, 2009 @ 04:31 PM

    It depends on whether brynary ever merged my branch or implemented an alternate fix; I know he'd pulled my branch and was going to run it against his test suite, but I'm not sure if he ever did.

  • Aleks Shamles

    Aleks Shamles March 6th, 2021 @ 05:04 AM

    Je ne pense pas que la vieillesse soit un obstacle majeur. On peut dire que l'achat d'un ce site est un excellent choix pour les personnes qui peuvent lutter contre leurs problèmes non seulement par des mots. Après ce remède, votre femme sera très surprise!

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Ruby Acceptance Testing for Web applications.

Shared Ticket Bins

People watching this ticket