Skip to content

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

Closed
wants to merge 1 commit into from
Closed

Conversation

0xrofi
Copy link

@0xrofi 0xrofi commented Mar 31, 2017

This pull-req fix the problem that Fixed parameters can be overwritten with Get parameters.

Here is the reproduction scenario, routes

GET     /hoge            App.Hoge("fixed")

and controller

func (c App) Hoge(param string) revel.Result {
	fmt.Println(param)
	return c.Render()
}

and url

http://localhost:9000/hoge?param=get

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.

@notzippy notzippy added status-ready Ready to implement type-bug labels Mar 31, 2017
@notzippy
Copy link
Collaborator

Can you update params_test.go with a test case for this situation ? thanks..

@pedromorgan
Copy link
Member

Confused.. The fixed should overwrite all other params, but all other params need to be arrays ?

@0xrofi
Copy link
Author

0xrofi commented Apr 3, 2017

@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, Route parameters and Form parameters also can be overwritten with Query parameters. (Sorry for Get in I said above, it's a mistake of Query)
problem of #1095 isn't fixed yet and I think this patch fixes it.

If all params(Query, Form, Route and Fixed) need to append to array, I think that it should change the order of appending parameters (it's discussed in #503 ).

@notzippy
Copy link
Collaborator

notzippy commented Apr 4, 2017

Would this be better ? So highest gets the first slot but lower ones can be promoted...

	for k, v := range p.Fixed {
-		values[k] = append(values[k], v...)
+		values[k] = append(v, values[k]...)
 	}

@0xrofi
Copy link
Author

0xrofi commented Apr 4, 2017

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...)
        }

@notzippy
Copy link
Collaborator

notzippy commented Apr 4, 2017

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.

	// ?query vars are first
	for k, v := range p.Query {
		values[k] = append(values[k], v...)
	}
    
	// form vars append so query first, form second
	for k, v := range p.Form {
		values[k] = append(values[k], v...)
	}
    
	// :/path vars overwrite and any above variables over written with path
	for k, v := range p.Route {
		values[k] = v 
	}
    
	// fixed vars overwrite, finaly rewrite all above values with the fixed ones
	for k, v := range p.Fixed {
		values[k] = v 
	}

@notzippy notzippy requested a review from brendensoares April 4, 2017 21:20
@pedromorgan
Copy link
Member

@notzippy Yes that is what we want.. All query/post etc vars appended.. BUT a fixed one is CONST ;-) ta.. commit it bro...

@pedromorgan
Copy link
Member

Need to update manual to explain this problem.. and also a new CORS chapter.. ;-)

@notzippy
Copy link
Collaborator

notzippy commented Apr 5, 2017

@pedromorgan I think with this PR #1171 you may need to rewrite the manual :-P
I incorporated this change in there..

@notzippy notzippy added status-doing Implementation has started and removed waffle: ready labels May 3, 2017
@notzippy notzippy added this to the v0.16 milestone May 5, 2017
@brendensoares
Copy link
Member

Closing in favor of the above defined hybrid approach to be released with PR #1171. Thanks for everyone's effort on this!

@brendensoares brendensoares removed the status-doing Implementation has started label May 31, 2017
nbramblett added a commit to yext/revel that referenced this pull request Jan 3, 2023
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status-ready Ready to implement type-bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants