Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

content types need to be ranged over in case of ones with a ';' in them #234

Merged
merged 7 commits into from
Feb 7, 2017

Conversation

markbates
Copy link
Member

No description provided.

Copy link
Contributor

@philipithomas philipithomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only issue I see is that this takes the complexity from constant time to quadratic. If this is being run on every request, that could start to be a problem (or an attack vector - e.g. sending Content-Type: ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; with a larger DDOS).

If you agree that it's a problem, I propose splitting on the semicolon and just using the first substring. That keeps things constant-type.

@markbates
Copy link
Member Author

Ah, yes, good point. Didn't even cross my mind. Good catch.

if b, ok := binders[strings.TrimSpace(c)]; ok {
return b(d.Request(), value)
}
cts := strings.Split(ct, ";")[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this for empty string "ct"? I'm afraid it will panic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting on an empty string returns a slice that has 1 empty string element in it:

https://play.golang.org/p/_y55t5_kC9

Still, I'll wrap it anyway, just to prevent future peeps from questioning at it.

Copy link

@vasukolli vasukolli Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if content type contains charset like "application/json; charset=utf-8", then charset will not be considered.

@philipithomas
Copy link
Contributor

@markbates I opened a PR into this branch to check the more specific scenario that I was thinking of. It looks like it did not error, so I learned something today :-)

#238

@markbates markbates merged commit 891616e into master Feb 7, 2017
@markbates markbates deleted the fix-binders branch February 7, 2017 16:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants