Fixed in 87535dbcf175ed115ae67f61192223d199428543.

It's highly ill-advised of course, but yes, it's legal.

There isn't a fatal issue with conflicting with future extension because the spec wouldn't allow that sort of addition without the binding itself being a new one, which there isn't any guarantee is possible to implement at the same endpoint as an existing one.

I doubt the SP even checks for pathological errors like duplicate parameters.

I don't know why we are clearing those there. It's been that way since the original impl in v2 in 2006.  I personally can't find anything in the spec that suggests that query params in the supplied endpoint URL are disallowed.  I'm speculating that the original implementer might have misinterpreted this text in SAML Bindings 3.4.4.1 line 606-607:

... Any other content in the original query string is not included and not signed.

However that is in the context of signature generation only, and so means merely that other query string content is not included in the signature, and therefore not signed.

I'll let Scott weigh in just in case I'm missing something, and to confirm what the SP does. But I think I agree at this point that this is a bug.

In fixing I think we do need to disallow the query param names which are already reserved for use in this binding, namely:

SAMLEncoding
SAMLRequest
SAMLResponse
RelayState
SigAlg
Signature

So those would be cleared, but any others would be allowed through.