Ignore query parameters when matching url in rules.

[stszap]

It seems that oathkeeper compares urls without stripping GET parameters. For example, a rule with

    "match": {
        "url": "http://test.com/test/",
        "methods": ["GET"]
    }

matches http://test.com/test/ but doesn't match http://test.com/test/?param=value. Comments say:

it compares the full request URL (e.g. https://mydomain.com/api/resource) without query parameters of the incoming request

So I guess it's a bug? According to the docs .String() method uses scheme://userinfo@host/path?query#fragment pattern so I tried to fix it by simple Sprintf() and added a test.

Checklist

• I have read the contributing guidelines
• I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
  vulnerability, I confirm that I got green light (please contact hi@ory.sh) from the maintainers to push the changes.
• I signed the Developer's Certificate of Origin
  by signing my commit(s). You can amend your signature to the most recent commit by using git commit --amend -s. If you
  amend the commit, you might need to force push using git push --force HEAD:<branch>. Please be very careful when using
  force push.
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation within the code base (if appropriate)
• I have documented my changes in the developer guide (if appropriate)

[aeneasr]

Nice catch. Maybe we should make this configurable? Just merging this could leave people vulnerable that think it's matching query params? What do you think?

[stszap]

Well, this behavior was only mentioned in the comments and the docs are not clear about it. I can try to add "ignore_query_params" boolean option to "match" section if security of current users is a concern.

[aeneasr]

Yeah, I think that would be a good idea!

[aeneasr]

So it would be like this then:

    "match": {
        "url": "http://test.com/test/",
        "methods": ["GET"],
        "ignore_query_params": true
    }

[aeneasr]

Or hm, I mean it doesn't really make sense, it's almost impossible to match query params using regex. Maybe it would make more sense like this:

"match": {
    "url": "http://test.com/test/",
    "methods": ["GET"],
    "query_params": {"foo": "<.+>", "bar"}
}

What's your take on that?

[stszap]

I think that verifying individual parameters may be too much. It couples an application with oathkeeper and may require pretty complex syntax/options. There can be users who only want to validate certain parameters and just ignore the rest, and other users who want to validate every parameter and deny everything else. Some of params can be arrays and some can be strings. I don't have any concrete arguments agains it but my gut feeling is that get parameters should be left for an application to deal with and there should be a way for oathkeeper to ignore them.

[aeneasr]

Yeah, that makes sense! So let's keep the PR as is and have it ignore query params completely for now. Thank you!