Change the key from a query paramter to a HEADER by misilot · Pull Request #328 · maxtepkeev/python-redmine · GitHub
Skip to content

Change the key from a query paramter to a HEADER - #328

Merged
maxtepkeev merged 1 commit into
maxtepkeev:masterfrom
misilot:misilot-patch-1
Mar 3, 2024
Merged

maxtepkeev merged 1 commit into
maxtepkeev:masterfrom
misilot:misilot-patch-1

Conversation

@misilot

@misilot misilot commented Aug 11, 2023

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Owner

@ricardobranco777

Copy link
Copy Markdown

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

misilot commented Sep 19, 2023

Copy link
Copy Markdown
Contributor Author

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

ricardobranco777 commented Sep 20, 2023 •

Copy link
Copy Markdown

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

ricardobranco777 commented Sep 26, 2023 •

Copy link
Copy Markdown

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Oct 2, 2023
https://build.opensuse.org/request/show/1114261
by user dirkmueller + anag+factory
- Add 328.patch to workaround "API key leakage on exception"
  described in boo#1215722
  maxtepkeev/python-redmine#330
  maxtepkeev/python-redmine#328
@maxtepkeev
maxtepkeev self-requested a review March 3, 2024 10:05
@maxtepkeev maxtepkeev self-assigned this Mar 3, 2024
@maxtepkeev maxtepkeev assigned misilot and unassigned maxtepkeev Mar 3, 2024
@maxtepkeev

Copy link
Copy Markdown
Owner

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.

@maxtepkeev
maxtepkeev merged commit c656cd9 into maxtepkeev:master Mar 3, 2024
@ricardobranco777

Copy link
Copy Markdown

Thank you! Will you make a new release?

@maxtepkeev

Copy link
Copy Markdown
Owner

maxtepkeev added a commit that referenced this pull request Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants