Skip to content
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

Pass raw: true to prevent Ruby de/serialization. This is to make it po... #118

Merged
merged 1 commit into from
Apr 16, 2015

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Mar 15, 2015

...ssible

to implement something like:

store.write(key, 0, :expires_in => expires_in)

See #113

… possible

to implement something like:

```store.write(key, 0, :expires_in => expires_in)```

See rack#113
@stanhu
Copy link
Contributor Author

stanhu commented Mar 15, 2015

Is it better to pass options to both read and write? I noticed the Dalli proxy code forced usage of raw mode. In addition, the increment method stores values in raw mode, so it seems consistent to use raw across the board.

@ktheory
Copy link
Collaborator

ktheory commented Apr 16, 2015

@stanhu: looks good. thanks.

ktheory added a commit that referenced this pull request Apr 16, 2015
Pass `raw: true` to prevent Ruby de/serialization. This is to make it po...
@ktheory ktheory merged commit 7dd9a9d into rack:master Apr 16, 2015
@ktheory
Copy link
Collaborator

ktheory commented Apr 16, 2015

@stanhu: Is this going to cause errors when deployed?

E.g., an app has a bunch of marshalled values in redis already. When they deploy this update, Rack::Attack will read them as raw values instead.

@stanhu
Copy link
Contributor Author

stanhu commented Apr 16, 2015

@ktheory, yes it is a potential issue. We could make it default to marshalled values and use the options to override this behavior. But there is an inconsistency already in that the increment function uses raw values and the read uses marshalled values, which is what caused me hours of debugging. Thoughts?

ktheory added a commit that referenced this pull request Apr 16, 2015
@ktheory
Copy link
Collaborator

ktheory commented Apr 16, 2015

@stanhu: ok, so the only time Rack::Attack reads keys is in Fail/Allow2Ban. And anyone using Fail/Allow2Ban with redis would already be getting these serialization errors.

So it seems like this is strictly an improvement; not a gotcha for any redis users. 😄

@stanhu
Copy link
Contributor Author

stanhu commented Apr 16, 2015

@ktheory: Right, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants