Change the key from a query paramter to a HEADER

[misilot]

Instead of having the Key being a query parameter in the url (which can shows up in webserver access logs), move it to the header.

https://www.redmine.org/projects/redmine/wiki/Rest_api#Authentication

[maxtepkeev]

Thanks for the PR, but I don't see the real benefit to be honest, because normally a Redmine and webserver are on the same server, meaning under the control of the same admin, if we imagine that the server is hacked and the attacker gained control on the server, then, probably yes, but having a key in the logs will be the least serious problem of the server admin in this case.

What bothers me more is that with this change we can break lots of Redmine servers that are behind the webserver like nginx and others, because in most cases those customs headers are required to be in the web server configuration files to be proxied to Redmine.

And the reason I went with the key and not the header in the first place, was because a header was only introduced in Redmine 1.1.0.

Thoughts ?

[ricardobranco777]

Thoughts ?

I opened #330 because the API key may leak in publicly available CI's like Github Actions, etc. when logging exceptions. At least we should mitigate this even if there's a workaround.

[misilot]

Would adding this behind a feature flag be an acceptable change? If you were to make it the default, I think a v3.0.0 of the library would make sense as it could be a breaking change for systems that do not pass the header. Though, we run redmine behind Apache and I have other webservices using the HEADER without having to do anything special to pass the header to the backend.

[ricardobranco777]

I ended up using this workaround thanks to this PR:

client = Redmine(url=url, raise_attr_exception=False, **creds)
key = client.engine.requests['params'].get('key')
if key is not None:
    client.engine.requests['headers']['X-Redmine-API-Key'] = key
    del client.engine.requests['params']['key']

Our server is also behind Apache and no issues.

[ricardobranco777]

Patch included in openSUSE's python3-redmine:

https://build.opensuse.org/package/view_file/devel:languages:python/python-python-redmine/328.patch?expand=1

[maxtepkeev]

After investigating this a bit more I found out that nginx (which I was worried about as Apache does the header forwarding by default) also has the header forwarding turned on by default for quite a while (proxy_pass_request_headers setting) so we should be pretty safe introducing this change as is without breaking backwards compatibility for most of the setups.

Sorry about the delay.

[ricardobranco777]

Thank you! Will you make a new release?

[maxtepkeev]

@ricardobranco777 Absolutely, I just wanted to check a few things here and there first, but will surely do a release this month.