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

Sinatra 1.3.2 adds 'non request' params to params objects with some routes #453

Closed
mattetti opened this issue Jan 12, 2012 · 11 comments
Closed
Milestone

Comments

@mattetti
Copy link

When using routes such as: /users/:id 2 'non request' params get injected in the params object (captures and splat to be more exact). That broke by param verification code which is checking the params against specific rules.

Could you please consider moving these values from the params hash?

Thanks

@jamiehodge
Copy link
Contributor

This also crops up with Rack::Parser, which doesn't namespace its result. Various parsers choke on the arrays it finds in params.

I don't imagine this is a simple fix, as splats etc should be in params.

@jamiehodge
Copy link
Contributor

Would it not be enough to include non-empty splats and captures?

@mattetti
Copy link
Author

@jamiehodge sounds to me that doing that would mean that it would then only sometimes break things instead of always breaking. I'd prefer to not have the splats and captures in the params at all.

@yb66
Copy link

yb66 commented Jan 19, 2012

I've just found this problem too. I'd definitely prefer not to have splats and captures in the params, as per the behaviour in 1.3.1.

@dorkusprime
Copy link

Agreed . . . this is kind of a nasty pollution of expected params. What would happen if, for instance, I send in a 'captures' or 'splat' parameter from a GET or POST request?

answer: they get completely stepped on.

http://localhost:4567/api/test/test?splat=1&captures=1&a=1

yields this params object:

{"splat":[],"captures":["test","test"],"a":"1"}

@Phrogz
Copy link

Phrogz commented Jan 31, 2012

+1 This just bit me. Although my code mostly ignores the extra params, it doesn't do well with non-string values (I was calling .strip! on every item, and the arrays broke that). It seems wrong to put these items in @params.

@dorkusprime That's really unfortunate.

@rkh
Copy link
Member

rkh commented Mar 7, 2012

Splat and captures have "sometimes" been in the params ever since 0.2.0, possibly earlier, so removing them would not be possible before 2.0. If you need access to the GET/POST for validation or because they are named splat, consider using request.params, request.GET or request.POST.

@rkh rkh closed this as completed Mar 7, 2012
@pheino
Copy link

pheino commented Oct 12, 2012

When using request.params paring params out of the URL don't seem to work once we upgraded to this version of Sinatra.

/policies/1/accept.xml

Does anyone know where this code lives or as to why request.params don't return the RESTFUL params? This used to work fine.

@rkh
Copy link
Member

rkh commented Oct 12, 2012

request.params never contained these, only params. Should we add them to request.params? I am not sure about that yet, since up until now request has been independent of the block you were in.

@rkh rkh reopened this Oct 12, 2012
@pheino
Copy link

pheino commented Oct 13, 2012

Probably not if Sinatra is going to inject it.

OR remove the splat and captures as that is really my pain point.

/policies/1/accept.xml

Policy.find(params[:id])

param[:id] # expected

param[:captures][:id] # not expected and causes API contract to be violated since we strictly enforce params to exactly what the API allows/expects any unexpected params result in an exception

@rkh rkh closed this as completed Jan 26, 2013
fgo pushed a commit to fgo/sinatra that referenced this issue Oct 3, 2013
@CloCkWeRX
Copy link

This or something very similar appears to have cropped up in 2.0.0 (params regularly contains 'captures' again); I'm unclear if it was intentional or a regression of this issue; and the only time its mentioned is in the changelog for 0.9.0(!) as being added in... which this bug disagrees with.

If its intended (http://www.sinatrarb.com/intro.html suggests yes); would it be possible to document in the changelog/readme?

danieldreier pushed a commit to danieldreier/sinatra that referenced this issue Nov 10, 2017
sefton-ntech pushed a commit to sefton-ntech/sinatra that referenced this issue Apr 14, 2023
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

No branches or pull requests

8 participants