Re: [PATCH] showHTMLTitle.nse - bugfix and a few improvements

[David Fifield <david () bamsoftware com>]

On Tue, Sep 30, 2008 at 10:53:34PM +0100, jah wrote:
> On 30/09/2008 21:08, David Fifield wrote:
> > Two different mechanisms for getting the redirected page are used. With
> > an absolute URI, it's
> >         data = http.get_url( loc )
> > and with a relative URI it's
> >         data = http.get( host, port, loc )
> > Could that be simplified, maybe by having the two cases assign temporary
> > variables representing the new host, port, and loc, then having a common
> > http.get outside of the if block? It doesn't seem right to parse the
> > URL, reconstruct it, and have http.get_url parse it again.
> >   
> I'll have a go at that.  I think it might result in more code, but we'll
> see.

I was thinking something like

        local url = url.parse( data.header.location )
        local redir_host
        -- follow ONE redirect if host is not some other host.
        if type( url.host ) == "string" and url.host == host.targetname or url.host == ( host.name ~= '' and host.name 
) or url.host == host.ip then
                -- Absolute redirect.
                redir_host = url.host
        elseif type( url.host ) ~= "string" and type( url.path ) == "string" and url.path ~= "/" then
                -- Relative redirect (not HTTP/1.1-compliant).
                redir_host = host
        end
        if redir_host then
                local loc
                loc = ( ( type( url.path ) == "string" and url.path) or "/" )
                .. ( ( type( url.query ) == "string" and ("?%s"):format( url.query ) ) or "" )
                data = http.get( redir_host, port, loc )
                stdnse.print_debug("showHTMLTitle.nse: (%s) Default page is located at %s.", host.targetname or 
host.ip, loc)
        else
                stdnse.print_debug("showHTMLTitle.nse: (%s) Default page may be located at %s.", host.targetname or 
host.ip, data.header.location)
        end

That's actually quite a bit more code and not that much clearer. Its
benefit is that the long location concatenation and the HTTP retrieval
are done only once. I admit, the control logic isn't as trivial as I
thought it was and I had to try it a few times. I'll let you decide how
you would like to structure it.

> > What's the reason for this and similar changes?
> >
> > -       if data.status == 301 or data.status == 302 then
> > +       if type( data ) == "table" and type( data.status ) == "number" and tostring( data.status ):match( "30%d" ) 
> > and type( data.header ) == "table" and
> > type( data.header.location ) == "string" then
>
> My script writing seems to have evolved from testing for a nil value to
> being really explicit.  This as a result of often having to fix problems
> where an operation on a value breaks the script because the type of the
> value was unexpected.  It's almost second nature to me now.

I would rather have errors in NSE modules crash scripts. If someone were
to introduce a bug in http.lua that caused it to return, say, a number
instead of a table, checking for type(data) == "table" will hide the
error completely. The bug will be harder to discover because will appear
to be working, just because it's not throwing an error.

If NSE modules are returning mysterious types let's fix the modules.
Scripts shouldn't have to work around bugs in them. After calling qsort
one doesn't double-check that the array is sorted; one would only do
that if a bug were suspected in qsort.

That said, I think the general style of inline type checking you use:
        if type(x) == "number" and x > 0 then
is good style, when x could be something other than a number.

David Fifield

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org