Jump to content

almost done

jcombs_31

By jcombs_31
in Website Critique

 Share

Recommended Posts

AndyB Regular Member

AndyB

Posted
Posted
It's excellent. Three tiny things - the mouseOver color on the side menu links are a little light. Although I'm sure they exactly match a colour used elsewhere ...

A link to 'top of page' might be worthwhile on the longer pages.

And the other is a personal piece of weirdness. On the Citrus and 3D Landscape pages, the side menu ends with a sort of 'quote' graphic with a large point end that's directing me away from the content. Visually, it might be better to make it point back at the page content.
Link to comment
Share on other sites
steviewdr Member

steviewdr

Posted
Posted
Looks very well.

1 thing however, your Citrus page - There is a PERFECT crisp image of a citrus in the top image on the left. This is a perfect quality image.
When comparing the quality of this image to that on the index page - of the orange, the orange isn't of similar quality. I think you need a better orange image for the index page for the top image.

It is VERY sweet apart from that - a HUGE improvement on their current website ,-)

Oh -
http://www.a1acitrus-racing.com/new/landscape.php?id='858
http://www.a1acitrus-racing.com/new/citrus.php?id='14
Be careful!!


-steve
Link to comment
Share on other sites
448191 Advanced Member

448191

Posted
Posted
[quote author=steviewdr link=topic=106792.msg427619#msg427619 date=1157361257]
Oh -
http://www.a1acitrus-racing.com/new/landscape.php?id='858
http://www.a1acitrus-racing.com/new/citrus.php?id='14
Be careful!!
[/quote]

I think he's already using mysql_real_escape_string, so above examples will never produce anything usefull for a hacker.

I tried a little hacking of my own, and although I'm not an expert by any standard, I did find that there is some small room for exploitation.

http://www.a1acitrus-racing.com/new/landscape.php?id=1%20or%201=1

Gives me all items. I know that's not very scary. ;D

Anything with quotes failed, but I was able to probe for the table name.

http://www.a1acitrus-racing.com/new/citrus.php?id=14%20OR%20citrus_prod.id%20IS%20NOT%20NULL

[quote]Could not run query: Unknown table 'citrus_prod' in where clause[/quote]

I didn't find it though..

When trying to get table names, I did get some of the MySQL connection (user/host) information:
http://www.a1acitrus-racing.com/new/citrus.php?id=14%20UNION%20SELECT%20TOP%20TABLE_NAME%20FROM%20INFORMATION_SCHEMA.TABLES

[quote]Could not run query: SELECT command denied to user: 'jcombs31@linhost114.mesa1.secureserver.net' for table 'TABLES'[/quote]

Any subqueries I tried all failed.

All of this isn't very concerning, but I would still recommend you validate the string in $_GET['id'] to be numeric. That will eliminate any room for exploitation there might be.

Edit: Just a question, did you secure your contact form against email header injection? I would've tried it out, but didn't want to bother your client with obnoxious emails...
Link to comment
Share on other sites
jcombs_31 Advanced Member

jcombs_31

Posted
  • Author
Posted
[quote author=448191 link=topic=106792.msg427657#msg427657 date=1157367972]
[quote author=steviewdr link=topic=106792.msg427619#msg427619 date=1157361257]
Oh -
http://www.a1acitrus-racing.com/new/landscape.php?id='858
http://www.a1acitrus-racing.com/new/citrus.php?id='14
Be careful!!
[/quote]

I think he's already using mysql_real_escape_string, so above examples will never produce anything usefull for a hacker.

I tried a little hacking of my own, and although I'm not an expert by any standard, I did find that there is some small room for exploitation.

http://www.a1acitrus-racing.com/new/landscape.php?id=1%20or%201=1

Gives me all items. I know that's not very scary. ;D

Anything with quotes failed, but I was able to probe for the table name.

http://www.a1acitrus-racing.com/new/citrus.php?id=14%20OR%20citrus_prod.id%20IS%20NOT%20NULL

[quote]Could not run query: Unknown table 'citrus_prod' in where clause[/quote]

I didn't find it though..

When trying to get table names, I did get some of the MySQL connection (user/host) information:
http://www.a1acitrus-racing.com/new/citrus.php?id=14%20UNION%20SELECT%20TOP%20TABLE_NAME%20FROM%20INFORMATION_SCHEMA.TABLES

[quote]Could not run query: SELECT command denied to user: 'jcombs31@linhost114.mesa1.secureserver.net' for table 'TABLES'[/quote]

Any subqueries I tried all failed.

All of this isn't very concerning, but I would still recommend you validate the string in $_GET['id'] to be numeric. That will eliminate any room for exploitation there might be.

Edit: Just a question, did you secure your contact form against email header injection? I would've tried it out, but didn't want to bother your client with obnoxious emails...
[/quote]

Thanks for the feedback.  How would you suggest securing the email for against header injection?

[quote author=bob_the _builder link=topic=106792.msg427558#msg427558 date=1157348610]
Hi,

Just 1 thing ... your contact form, it clears the form when submited with missing fields, you should echo the post data in the fields so they dont have to retype the data if they miss a field out.

Nice job!
[/quote]


I know, didn't finish the contact form yet, just wanted design critiques mostly, but I'll finish that up today probably.

[quote author=AndyB link=topic=106792.msg427471#msg427471 date=1157333854]
It's excellent. Three tiny things - the mouseOver color on the side menu links are a little light. Although I'm sure they exactly match a colour used elsewhere ...

A link to 'top of page' might be worthwhile on the longer pages.

And the other is a personal piece of weirdness. On the Citrus and 3D Landscape pages, the side menu ends with a sort of 'quote' graphic with a large point end that's directing me away from the content. Visually, it might be better to make it point back at the page content.
[/quote]

Thanks Andy, nice subtle point about the pointer.
Link to comment
Share on other sites
448191 Advanced Member

448191

Posted
Posted
[quote author=jcombs_31 link=topic=106792.msg427762#msg427762 date=1157381588]
Thanks for the feedback.  How would you suggest securing the email for against header injection?
[/quote]

Validate. All fields other than the body cannot contain "\r", "\n", "%0D" or "%0D". I don't believe there is a way to inject malicous MIME stuff into the body if the other fields are secure, since php mail() already created the top-level part of the email. Make sure you disable multiparts by including a "Content-Type: text/plain;" header. I could be wrong, so maybe you should check for 'Content-Type' in the body. Make sure there is no HTML or javascript in there, to be sure.
Link to comment
Share on other sites
jcombs_31 Advanced Member

jcombs_31

Posted
  • Author
Posted
I think I fixed any problems you guys mentioned, with the exception of the item andy mentioned below the sub-menus. (I"m a little lazy for that now).  Any final comments?  Just a couple small things I have to finish, such as the slideshow on the racing pages, and a small CMS for them to update inventory.
Link to comment
Share on other sites
steviewdr Member

steviewdr

Posted
Posted
Just a few things:

The graphic saying "Delivery Available" - I think you should make it a link to a "Delivery" section with info on the rates etc. IMO.

For the Racing - and Car specifications - I would prefer to see a graphic of a racing car, rather than the driver again.

Thats about it. Looking forward to see the image gallery.

-steve
Link to comment
Share on other sites
jcombs_31 Advanced Member

jcombs_31

Posted
  • Author
Posted
Stevie, they want everyone to call for rates for the most part because it will vary.  I somewhat agree about the specs page, but the car image would not stretch like the image of him, but I'll maybe try it out and see how it looks.

I don't think the image gallery will be anything special, I'm going to try to piece it together from my own image gallery that i created.
Link to comment
Share on other sites
steviewdr Member

steviewdr

Posted
Posted
The image gallery ->

You need to specify a size for the main Div for the photo. It works PERFECT when the images are cached by my browser. BUT going through the image gallery for the first time (delete your temp internet files and see) the image page jumps up and down when loading the image.

Your image gallery link on http://www.a1acitrus-racing.com/new/racing.php?id=kids is broken.

Coming along nicely.

-steve
Link to comment
Share on other sites
This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
 Share
Go to topic listing
×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.