Add strict mode

[powerman]

While for most apps loose decoding is acceptable (and may be even preferable for speed), some require strict checking of form params (for ex. OAuth2 spec require this), and some other may like to enable it to make it easier to catch some bugs (like sending param with typo in name which now gets ignored by default and such bug may not be noticed for some time).

Returning new errors triggered by strict mode will require minor API changes:

• Decode() should return new error type(s) in addition to InvalidDecoderError and DecodeErrors
• probably Mode should be changed to use 1<<iota>>1 and have few more values added, which can be ORed with existing values to (partially) enable strict mode

Here is complete (I hope) list of changes required for strict mode:

• error on unknown param (related to Support DisallowUnknownKeys option #27)
  • including param matching real, but not qualified enough field name:
    • struct without .field (in case custom handler not registered)
    • map without [key]
    • array with out-of-bound [index]
    • too many params for array field
  • error instead of panic on field name with unmatched brackets (related to Decode shouldn't panic #7)
• error on decoding multiple values to same scalar
  • multiple values for non-slice/array field Update: (except []byte)
    • also pointer to slice/array (if such types are supported)
  • Update: including multiple values for same array[index] or map[key], in case this array/map doesn't have values of slice/array type
• error on decoding no values to non-pointer/slice/array field tagged ",required"

I'm going to implement (at least, partially) this for my service, but implementing in a separate package will not be effective (because I'll have to duplicate existing functionality of form to reflect data structure, check field types and tags) and probably will be incomplete (not sure I can detect registered custom handlers for whole struct).

If you like the idea and open to discussing related PR then I'll try to implement this inside form and send a PR.

[powerman]

First two items in above list cover case when decoder ignore existing values, last item cover case when decoder ignore missing value.

First item is special because there is no way to tie error to known field name. As there is a chance client will show only errors for known field names (often happens when errors are rendered near related field in form) plus at least one general error (which may happens anyway for any reason - from r.ParseForm() to business-logic after decoder), so returning DecodeErrors with unknown to client field names doesn't looks like a good idea - thus we need to enable Decode() to return some new error type.

I don't think we need custom error type here: at a glance all strict-mode errors are client bugs (or power user changed params in url manually), so there is no UX needs in returning all errors at once for convenience - we can return just one (detected first) error, so it looks like errors.New() is good enough for all these errors.

If we'll replace InvalidDecoderError with panic (see #7 (comment) for reasoning) then Decode() will return either DecodeErrors or *errors.errorString, so handling returned error won't be more complicated than now.

[deankarn]

Hey @powerman, first off love the enthusiasm!

I do have a lot of questions and recommendations, but am super busy so will do a quick brain dump now and try to formalize later.

Once I get to issue #27 that should solve the unknown fields.
Not looking to add validation like rules to this off the top of my head.
As for the rest why can't you use go-playground/validator for the verification of field lengths and such.

[powerman]

Once I get to issue #27 that should solve the unknown fields.

To me it looks like people try/want to detect unknown fields because they think they'll get strict validation of request parameters this way, but checking only for unknown fields won't detect all possible issues with request parameters, while solution proposed here looks complete and should do the trick.

Not looking to add validation like rules to this off the top of my head.

What do you mean?

As for the rest why can't you use go-playground/validator for the verification of field lengths and such.

validator runs after form and have no idea about existence of original url.Values. So, in case of decoding into field Name string validator will see "" in all these cases:

• r.Form["Name"] does not exists (absence of possibly required parameter)
• r.Form["Name"]=[]string{""} (parameter is present, just have empty value, which may be OK in case we require parameter to be provided but don't require it to have any specific - like non-empty - value)
  • this is why I proposed to introduce tag form:",required" in addition to validate:"required": first check for parameter existence, second for non-zero value
• r.Form["Name"]=[]string{"","second"} (parameter is provided multiple times, but all values but first was just ignored)

Missing required parameter is not always can be treated the same as parameter provided with empty value, for ex: OAuth2 require parameters to exists even if they have empty values; absent parameter may mean "use default value" while present parameter with empty value may mean "use empty string instead of default value". And while it's possible to distinguish between these cases by using pointer field like Name *string it's inconvenient.

Having non-slice/array parameter provided two times Name=one&Name=two may be a sign of attack (when hacker manage to inject own params into url before app has appended real params to that url), this is why this case is explicitly disallowed by OAuth2. To detect such case with validator we'll have to make field Name []string with tag like validate:"eq=1,dive,…(real validation rules here)…", which is inconvenient too.

[deankarn]

Thanks for clarifying the validation, it's less data validation and more attack and threat detection.
I definitely want to integrate some if not all of these ideas, I just have to think of how I want to integrate the changes and not slow down performance.

As for the other issues you've opened, I'll try and get to them, especially the panic thank you for finding, I may have questions and not agree with all but will comment as I can just super busy!

Thanks for the help 😄

[powerman]

Here is initial version of strict validation: https://github.com/powerman/urlvalues
At a glance it should be compatible with all decoder's features except Custom Type registration (I didn't tried to implement this yet, may be trivial or not) and support of decoding into interface type (hard to implement and probably no real sense to have strict validation in this case at all).

There is benchmark in README, and while penalty is noticeable to me it looks acceptable (I didn't tried to optimize anything yet expect adding a couple of obvious caches).

[WineChord]

@powerman Hi, any updates on this thread? We are also encountering the issue of panicking when the input is malformed, but users prefer graceful degradation instead of panicking.

[powerman]

@powerman Hi, any updates on this thread? We are also encountering the issue of panicking when the input is malformed, but users prefer graceful degradation instead of panicking.

No. In last years I worked mostly on API services so I've used mentioned above https://github.com/powerman/urlvalues in just one production project about five years ago. It works well AFAIR, so you can try it.

[WineChord]

@powerman, it would be better to merge it into the go-playground/form repository to receive future updates. We cannot risk depending on a fork that may not be actively maintained in the future.

[powerman]

@powerman, it would be better to merge it into the go-playground/form repository to receive future updates. We cannot risk depending on a fork that may not be actively maintained in the future.

It's not a fork.

[Jecoms]

The "shouldn't panic" issue was closed, but I agree that malformed input should not cause a panic. Until then, implementers need to make sure and wrap this library's methods with a defer/recovery pattern.