New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for a few of the new HTML 5 form inputs #764

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Cirex

Cirex commented Jan 22, 2012

If this is an acceptable change I will come back and add the rest along with YARD doc when I have the time

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 22, 2012

Member

Yeah all looks good to me, nice to have support for the html5 helpers. I don't want to have so many undocumented methods though so I don't want to merge into master until we add yard docs. Also, before a release I will then add these to the padrino guides as well.

Member

nesquena commented Jan 22, 2012

Yeah all looks good to me, nice to have support for the html5 helpers. I don't want to have so many undocumented methods though so I don't want to merge into master until we add yard docs. Also, before a release I will then add these to the padrino guides as well.

@Cirex

This comment has been minimized.

Show comment
Hide comment
@Cirex

Cirex Jan 22, 2012

I should have it all YARDed by tomorrow. Is there any interest in adding CSRF protection to forms via rack-protection?

Cirex commented Jan 22, 2012

I should have it all YARDed by tomorrow. Is there any interest in adding CSRF protection to forms via rack-protection?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 22, 2012

Member

Great, thanks. If it was configurable, With regards to csrf protection, yeah I could see that being very useful.

Member

nesquena commented Jan 22, 2012

Great, thanks. If it was configurable, With regards to csrf protection, yeah I could see that being very useful.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 22, 2012

Member

Wow, I like it!

Regarding CSRF, we can make it pluggable (not a default)

Thank @Cirex

Member

DAddYE commented Jan 22, 2012

Wow, I like it!

Regarding CSRF, we can make it pluggable (not a default)

Thank @Cirex

@Cirex

This comment has been minimized.

Show comment
Hide comment
@Cirex

Cirex Jan 22, 2012

Done. I left out color, date, datetime, time, week, and month as I've been out of the loop for a long time and I don't know how well they're supported. But, this should be a good enough start.

Cirex commented Jan 22, 2012

Done. I left out color, date, datetime, time, week, and month as I've been out of the loop for a long time and I don't know how well they're supported. But, this should be a good enough start.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 22, 2012

Member

@Cirex thanks! I love that!

Member

DAddYE commented Jan 22, 2012

@Cirex thanks! I love that!

@Cirex

This comment has been minimized.

Show comment
Hide comment
@Cirex

Cirex Jan 22, 2012

I've had CSRF protection baked into my application for awhile:

enable :request_forgery_protection
post :register, protect: false do
   # request is not checked
end

post :register do
  # request is checked
end

You can also disable it and verify authentication tokens on a request by request basis:

disable :request_forgery_protection
post :register, protect: true do
  # request is checked
end

post :register do
  # request is not checked
end

Would this be an acceptable API? Or is it better off as a plugin?

Cirex commented Jan 22, 2012

I've had CSRF protection baked into my application for awhile:

enable :request_forgery_protection
post :register, protect: false do
   # request is not checked
end

post :register do
  # request is checked
end

You can also disable it and verify authentication tokens on a request by request basis:

disable :request_forgery_protection
post :register, protect: true do
  # request is checked
end

post :register do
  # request is not checked
end

Would this be an acceptable API? Or is it better off as a plugin?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 22, 2012

Member

I could see it either way.

If it was easy to add as a plugin, I am often in favor of keeping the padrino core lean in favor of optional plugins that can be used if needed. Especially when the feature is only useful to some subset of applications.

But some things are hard to implement as a plugin. If you feel that creating this as a gem could work alright, that would be interesting. If it's something much easier to add directly into core, I think I'd be ok with that to. Thoughts @DAddYE?

Member

nesquena commented Jan 22, 2012

I could see it either way.

If it was easy to add as a plugin, I am often in favor of keeping the padrino core lean in favor of optional plugins that can be used if needed. Especially when the feature is only useful to some subset of applications.

But some things are hard to implement as a plugin. If you feel that creating this as a gem could work alright, that would be interesting. If it's something much easier to add directly into core, I think I'd be ok with that to. Thoughts @DAddYE?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 22, 2012

Member

@Cirex by the way, thanks for putting this all together. The HTML5 stuff looks great.

Member

nesquena commented Jan 22, 2012

@Cirex by the way, thanks for putting this all together. The HTML5 stuff looks great.

@Cirex

This comment has been minimized.

Show comment
Hide comment
@Cirex

Cirex Jan 22, 2012

It already exists in my lib folder as a plugin, so it's just a matter of extracting it.

I agree with you in keeping the core minimal. However, I also feel Padrino should promote the best practices. CSRF is a pretty common gotcha when it comes to security. I think we should take steps to deal with that in the same way that your ORM escapes input to prevent SQL injection.

I'm fine with either way, but I think it would make a great addition to the core.

Cirex commented Jan 22, 2012

It already exists in my lib folder as a plugin, so it's just a matter of extracting it.

I agree with you in keeping the core minimal. However, I also feel Padrino should promote the best practices. CSRF is a pretty common gotcha when it comes to security. I think we should take steps to deal with that in the same way that your ORM escapes input to prevent SQL injection.

I'm fine with either way, but I think it would make a great addition to the core.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 23, 2012

Member

Merged in html 5 request to master for upcoming gem release. Closing. Feel free to submit the csrf support as a patch to core, presuming its not a lot of extra code bulk and there's good tests I think we can put it into core.

Another option is to make it a plugin and have it generated by default in new project's Gemfiles.

Member

nesquena commented Jan 23, 2012

Merged in html 5 request to master for upcoming gem release. Closing. Feel free to submit the csrf support as a patch to core, presuming its not a lot of extra code bulk and there's good tests I think we can put it into core.

Another option is to make it a plugin and have it generated by default in new project's Gemfiles.

@nesquena nesquena closed this Jan 23, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment