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

Don't escape parameters by default in included rack-protection (issue #310) #361

Merged
merged 1 commit into from
Sep 22, 2011

Conversation

lanej
Copy link

@lanej lanej commented Sep 22, 2011

Don't escape parameters by default in included rack-protection.

As @rkh claims in issue #310.

rkh added a commit that referenced this pull request Sep 22, 2011
Don't escape parameters by default in included rack-protection (issue #310)
@rkh rkh merged commit 9c4ac4c into sinatra:master Sep 22, 2011
@rkh
Copy link
Member

rkh commented Sep 22, 2011

👏

@astjohn
Copy link

astjohn commented Sep 22, 2011

Thanks for the quick fix.

@gordonk
Copy link

gordonk commented Nov 4, 2011

Small gotcha, if you define your own protection exclusions make sure you also include the one applied in the fix as your options hash overwrites the one in the fix.
e.g.
set :protection, :except => [:frame_options, :escaped_params] #include escaped_params

@ericskiff
Copy link

Woooooow, thank you @gordonk, that was exactly what was biting us in the butt.

I'm afraid this ambiguity will bite anyone who tries to set :protection, :except

The basic flow is this:
"Hmm, something weird is happening.
Oh, I see sinatra tries to save me from myself. Stop that please. (setting a specific :protection, :except)
Nice, that looks like it fixed it.
Wait, why are other things breaking now?
(swear, curse, google, find this page)

IMO, If I need to change one thing, that :except should affect only the parameter I'm specifically setting, not overwrite all the defaults.

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.

5 participants