emmett-framework / emmett

The web framework for inventors
BSD 3-Clause "New" or "Revised" License
1.06k stars 71 forks source link

URL validation causes weird bug in general validation #65

Closed GiantCrocodile closed 8 years ago

GiantCrocodile commented 8 years ago

Try these examples:

The following URLs fail on URL validation of forms: www.domain.tld domain.tld http://domain.tld http://www.domain.tld http://www.domain.tld/

This works: http://domain.tld/

After I tried this again - the working URL wasn't accepted a second time. So this check is inconsistent and weirdly at the same time when this URL validation passes, all other form errors aren't shown like all other fields (which are empty in this situation) are okay even if I force them to be filled in.

There must be some bug in the form validation.

GiantCrocodile commented 8 years ago

I just submitted the same URL multiple times without changing anything in the whole form: the site reloaded and resetted every input to default values/empty. No error shows.

This looks like there is some problem when you spam validation check on forms.

GiantCrocodile commented 8 years ago

Another addition while I test: Spamming the form is making it resetting and showing no validation errors after like 4 - 5 times resubmitting consequently. This is no specific URL validation bug but general validation it looks like.

gi0baro commented 8 years ago

Ok, this is pretty weird. Can you share a snippet of the code involved? Or eventually, tell me if is a model form or a standard form, if you have any other condition in your validation and which session handler are you using..

GiantCrocodile commented 8 years ago

This issue happens on an auto-generated model form. The only conditions in this model's validation are:

The url field is checked for url and presence.

I'm using SessionCookieManager. Nothing complex or unusual in code so I don't think sharing this will help. If you still need it because you can't reproduce it, I will do this ofc.

gi0baro commented 8 years ago

Again, this sound very weird. Unfortunately I can't look at it right now. Will do some tests tomorrow. I'm really sorry about that.

GiantCrocodile commented 8 years ago

Hey man, don't worry at all. You said this have bugs - I have to stand up for my decision because I did it. I decided to use your framework, you didn't force me.

I just find bugs when I develop my small project and trial and error different things. So I would expect this. The only issue would be real security issues to me but even then, no sensitive project so I can just patch it and be happy.

You provide incredible service, day and night, just for free for your epic framework. I will stick for sure with this if service stays like this.

I'm glad you published this. Thanks and I hope you have fun by working on it.

gi0baro commented 8 years ago

Hi @GiantCrocodile, thanks for your words. I've just finished some tests: I've basically cloned the bloggy example, added a website field to the User model, implemented the rw access in registration and profile and starting trying signing up. I ended up with the impossibility to get the validation passing using any of the combination you given. Even with the one you said was passing (http://domain.tld/), I get the Invalid URL message as output. And this is correct: the url validation of weppy takes care of verifying also the url has a valid top level domain, and .tld is just not the case. If you switch it, for example to .com, every combination will pass. If you don't want this behavior, you can use the generic mode, for example:

validation = {
    'website': {'is': {'url': {'mode': 'generic'}}}
}

But it will accept even things like http://foobar, because doesn't validate the presence of a tld.

Still, I'm not getting how you made the form passing the validation with http://domain.tld/. I can't even spamming the form. Are you sure the record is in the db?

Regarding the spamming of form issue: seems that the reset of the form values after a certain number of failed validations is related to the CSRF protection logic. Probably I should review the code a bit since it tries to keep the session clean and probably is causing this unwanted thing. But is just related to cases when you keep sending failing forms.

So, ending up: I don't see any bugs on url validation, and the bug you spotted on the forms should be fixed up on the next release, thank you for letting me know of that.

Regarding your code: I invite you to clean the cookies related to your application, or logout and re-login to restart with a clean session and try again the validation.

GiantCrocodile commented 8 years ago

I'm sorry for false explaination/misleading one: The form doesn't pass but if you try it often enough with wrong value the fields get resetted (no input, no error per field shown) so it look like it was submitted. Indeed, there is no real entry done in the database.

I simply spammed wrong form data by refreshing and my browser ask to send same post data again so I just hit enter. I did this while I changed some stuff in my form and so I was too lazy to refill it all time.

gi0baro commented 8 years ago

@GiantCrocodile I've opened #67 for this bug. Thank you again for the catch.

GiantCrocodile commented 8 years ago

@gi0baro http://domain.com/ should pass right? If yes, this isn't working for me. I also tried Wikipedia URLs and manual written URLs with valid top level domain endings. Any URL I try won't pass at all.

gi0baro commented 8 years ago

@GiantCrocodile I think you have something else that is altering the validation. Just tested:

Python 2.7.8 (default, Nov  6 2014, 14:53:57)
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.54)] on darwin
weppy shell on app: bloggy
>>> from weppy import Field
>>> u = Field(validation={'is': 'url'})
>>> u._make_field('u')
<weppy.dal.base.Field object at 0x103a0b250>
>>> u.requires
[<weppy.validators.consist.isUrl object at 0x103a0bc10>, <weppy.validators.basic.hasLength object at 0x103a0bc50>]
>>> u.validate('http://domain.com/')
('http://domain.com/', None)
>>> u.validate('foo.tld')
('foo.tld', <lazyT 'Invalid URL'>)

Have you added other conditions to validation?

GiantCrocodile commented 8 years ago
        "source": {
                        "presence": True,
                        "is": "url"
                        },

is the only validation for this field. Just a simple presence check and the problematic url check.

gi0baro commented 8 years ago

@GiantCrocodile with your conditions:

>>> u = Field(validation={'presence': True, 'is': 'url'})
>>> u._make_field('u')
<weppy.dal.base.Field object at 0x103a0bc90>
>>> u.requires
[<weppy.validators.basic.isntEmpty object at 0x103a0bc50>, <weppy.validators.consist.isUrl object at 0x103a0b250>, <weppy.validators.basic.hasLength object at 0x103a0bc10>]
>>> u.validate('http://domain.com/')
('http://domain.com/', None)
>>> u.validate('http://domain.com')
('http://domain.com', None)

Can you please open up weppy shell on your application an attach the output of:

from youarppname import db
db.YourModel.yourfield.requires
db.YourModel.yourfield.validate('http://domain.com/')
GiantCrocodile commented 8 years ago

@gi0baro the output is:

Python 3.4.3 (default, Oct 14 2015, 20:28:29) 
[GCC 4.8.4] on linux
weppy shell on app: test
>>> from test import db
>>> db.Question.source.requires
[<weppy.validators.basic.isntEmpty object at 0x7f826e735ba8>, <weppy.validators.consist.isUrl object at 0x7f826e735b00>, <weppy.validators.basic.hasLength object at 0x7f826e735b70>]
>>> db.Question.source.validate('http://domain.com/')
('http://domain.com/', <lazyT 'Invalid URL'>)

As you can see, the URL is being invalidated.

gi0baro commented 8 years ago

@GiantCrocodile Ok, seems python 3 related. Will investigate this and report to you back as soon I have news.

gi0baro commented 8 years ago

@GiantCrocodile this is actually fixed both on master and v0.5.4 just released. Sorry for the inconvenience.