-
Notifications
You must be signed in to change notification settings - Fork 1.4k
param map overwrite priority #1155
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
Conversation
Can you update params_test.go with a test case for this situation ? thanks.. |
Confused.. The |
@notzippy This situation is not only params but also include route, binder, controller. So I don't know how to write this situation's test case... sorry. @pedromorgan Because first element of array is used to controller's arguments. So just like as I said above, If all params( |
Would this be better ? So highest gets the first slot but lower ones can be promoted...
|
Alternatively I suggest here instead of this PR. // Copy everything into a param map,
- // order of priority is least to most trusted
+ // order of priority is most to least trusted
values := make(url.Values, numParams)
- // ?query vars first
- for k, v := range p.Query {
+ // fixed vars first
+ for k, v := range p.Fixed {
values[k] = append(values[k], v...)
}
- // form vars overwrite
- for k, v := range p.Form {
+ // :/path vars append
+ for k, v := range p.Route {
values[k] = append(values[k], v...)
}
- // :/path vars overwrite
- for k, v := range p.Route {
+ // form vars append
+ for k, v := range p.Form {
values[k] = append(values[k], v...)
}
- // fixed vars overwrite
- for k, v := range p.Fixed {
+ // ?query vars append
+ for k, v := range p.Query {
values[k] = append(values[k], v...)
} |
What about a hybrid approach ? Since fixed and route parameters should never be set using query or form values they always do a replace, and query and form parameters are appended together.
|
@notzippy Yes that is what we want.. All query/post etc vars appended.. BUT a fixed one is CONST ;-) ta.. commit it bro... |
Need to update manual to explain this problem.. and also a new CORS chapter.. ;-) |
@pedromorgan I think with this PR #1171 you may need to rewrite the manual :-P |
Closing in favor of the above defined hybrid approach to be released with PR #1171. Thanks for everyone's effort on this! |
Per the issue linked below, the previous change was insufficient for proper security. This fixes the priority entirely by having fixed params fully override query params, rather than appending. revel/revel#1155 (comment)
This pull-req fix the problem that Fixed parameters can be overwritten with Get parameters.
Here is the reproduction scenario,
routes
and controller
and url
exptected output:
fixed
but actual:
get
Issue #1095 and pr #1126 change the order of params priority, but insufficient.
So this pr is overwrite low priority params by higher priority params.