Option to redirect to exact service url

[pradtke]

Some clients will encode query parameters in their service url incorrectly.
If SSP/php's query builder utils are used then the service url is parsed,
and reconstructed differently then what is stored in the ticket. Sometimes
badly encoded parameters are lost.

Example: should space in a query param be encoded as '+' or '%20'

[tvdijen]

Code-wise I have no remarks, but not sure if we should jump through flaming hoops to facilitate bad client-behaviour...

[pradtke]

Thanks for the read through. As to whether the module should support this... we ( Cirrus ) need to have more parity between this module and the behavior of Shib CAS and Jasig CAS since we've transitioned customers from both of those to SSP, and this is one area where we've encountered issues. Also seems like other people have addressed it with their own local patches (https://groups.google.com/forum/#!searchin/simplesamlphp/cas|sort:date/simplesamlphp/2J7BxXsVfkk/zzRttynxBAAJ).

[tvdijen]

Is there a way we can detect this case and automate the fix, instead of adding a config flag?
If not, then let's merge it as is..

[pradtke]

I guess one could argue, why are we (in the HTTP utils) even parsing and re-encoding the provided url rather than just redirect back to it? For example if the client provided a valid query string with param=abc%20efg then our default behavior is to change that to param=abc+efg. Which is valid? Well, depends which RFC you're reading. It's ambiguous.
I originally thought this could be fixed during validation, by doing the same re encoding of the urls before comparison (much like the code removes the jsessionid path param). That would work fine for the above case, since the information is preserved. There is another case where the query parameter in the service url contains a '?', and the query parser in HTTP utils is not smart enough, and part of the query is dropped when we respond resulting in a url on the service that is missing a value. That can't be fixed at the serviceValidate time.

I suspect we will make this flag 'on' for all our deployments (eventually).

[pradtke]

I realized my fix doesn’t handle a #fragment correctly. I think I’ll switch to the sample code from the forum post I linked earlier.

…

On Thu, Aug 29, 2019 at 5:34 PM Tim van Dijen ***@***.***> wrote: Is there a way we can detect this case and automate the fix, instead of adding a config flag? If not, then let's merge it as is.. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#25?email_source=notifications&email_token=AAHDYRTISIRW7SBLIEBFLX3QHBFGDA5CNFSM4ISEHKZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5QAN6I#issuecomment-526386937>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHDYRXUCOJTIPIDLCFVZGDQHBFGDANCNFSM4ISEHKZQ> .

[tvdijen]

Well, we could consider adding an enc_type to the http_build_query call in HTTP::addURLParameters? Would that solve your problem? Perhaps @jaimeperez has a suggestion...

One thing I overlooked last night is that you're changing the default behaviour here by defaulting the new option to false.. We shouldn't do that..

[jaimeperez]

I share your pain @pradtke, but at the same time it makes me slightly nervous to introduce complexity just to deal with entities not working properly...

[pradtke]

I think my describing this as buggy client was a bad idea.... buggy client is not the only cause.
It also can be caused by bugs in HTTP::addURLParameters()
For example:

1. If a valid client request contains a query parameters a=val1&a=val2 with multiple values for a key, then HTTP::addURLParameters() only preserves one of those values.
2. If a client submits a request with a service url containing a fragment (#), then HTTP::addURLParameters() adds the ticket to the fragment and not to the query parameters which come before the fragment seems to remove the fragment.
3. the ambiguous ways a ' ' can be encoded, as described earlier

[tvdijen]

1 & 2 we should be able to fix..
3rd is depending on the RFC you follow.. We could add an extra parameter to HTTP::addURLParameters to make it a bit more flexible in this matter.. However, we would have to detect what RFC was used on the received URL..

[pradtke]

We've been analyzing our CAS service urls the last two weeks and there is one more case where there is ambiguity in re-encoding, specifically if percent encodings are case sensitive. Some .net based clients use lowercase percent encodings, while if php re-encodes the query parameter it uses upper case, resulting in a mismatch.

[pradtke]

I've rebased changes on master, adjusted some of the test and comments to reflect encoding issues with lower case hexadecimal and multiple query params with the same name (not just badly encoded urls). I also removed the option to use the old behavior (have SSP re-encode the url) since that encodes problematically in about 1% of our traffic (about 500,000 cas logins across ~10 schools since the PR was created). The newer way (similar to Jasig's CAS server's way) handle the traffic without issue. @tvdijen If your feelings on having this merged in have changed since the summer, let me know and I'll do a squash and merge. If you still think it shouldn't be merged then I'll leave it here as a PR so that anyone else encountering similar issues can download and apply as a patch. thanks

[tvdijen]

I'd like to know what @jaimeperez feels about solving 1 & 2 in SSP..

[tvdijen]

How relevant is this in 2021? Not sure why the tests fail either

[pradtke]

I reviewed our traffic from today and both Banner and Kuali still send CAS service urls that are "weird" in some way, and require the workaround. I'm unsure if this is because those apps still run the same version as in 2019, or an updated version with the same bugs.

We run all our customers with this patch (or some variation of it) enabled.

I can look at the cause of the PR checks failure, but probably won't have the time till January.