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

Mitigate BREACH attack #1171

Merged
merged 1 commit into from Aug 17, 2016

Conversation

Projects
None yet
3 participants
@jkowens
Member

jkowens commented Aug 17, 2016

Replaces sinatra/rack-protection/pull/115.

@zzak so instead of adding a new utility module, I just added the class methods to AuthenticityToken. I wasn't able to make use of the #random_string method because it doesn't allow the length to be set and it doesn't return a base64 encoded value.

Let me know if you have any more suggestions to clean this up. Thanks!

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Aug 17, 2016

Member

Yeh, I'm ok with not changing random_string but the point I was trying to make is that we've put utility methods under that class before, so it's fiiiiine ;)

Member

zzak commented Aug 17, 2016

Yeh, I'm ok with not changing random_string but the point I was trying to make is that we've put utility methods under that class before, so it's fiiiiine ;)

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Aug 17, 2016

Member

Also, as much as I love cribbing code and this looks good to me.

I'm no expert, and would feel at ease if I could get @tarcieri to give this a quick review before merging.

<3 <3 <3 <3

Member

zzak commented Aug 17, 2016

Also, as much as I love cribbing code and this looks good to me.

I'm no expert, and would feel at ease if I could get @tarcieri to give this a quick review before merging.

<3 <3 <3 <3

@tarcieri

This comment has been minimized.

Show comment
Hide comment
@tarcieri

tarcieri Aug 17, 2016

Looks good on cursory inspection

tarcieri commented Aug 17, 2016

Looks good on cursory inspection

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Aug 17, 2016

Member

@tarcieri Thank you!

@jkowens Ok, after further review and compare with Rails implementation, I think we're good to go.

I will merge this and start to prepare pre-releases, hopefully we can identify any issues before final goes out some time after that! <3

Member

zzak commented Aug 17, 2016

@tarcieri Thank you!

@jkowens Ok, after further review and compare with Rails implementation, I think we're good to go.

I will merge this and start to prepare pre-releases, hopefully we can identify any issues before final goes out some time after that! <3

@zzak zzak merged commit 6aeaf64 into sinatra:master Aug 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment