Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cherry-picking accident in recent auth bug fix #325

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

jmarshall
Copy link

@jmarshall jmarshall commented Jan 3, 2024

The recently cherry-picked CVE-2023-51663 fix (PR #323) included refactored code dependent on recent upstream code that was not cherry-picked. This caused:

{"severity":"ERROR","levelname":"ERROR","asctime":"2024-01-03 01:59:09,079","filename":"auth.py",
"funcNameAndLine":"callback:341","message":"oauth2 callback: could not fetch and verify token",
"exc_info":"Traceback (most recent call last):\n
File \"/usr/local/lib/python3.9/dist-packages/auth/auth.py\", line 335, in callback\n
    flow_client = request.app[AppKeys.FLOW_CLIENT]\n
NameError: name 'AppKeys' is not defined",
"hail_log":1}

Context: https://centrepopgen.slack.com/archives/C030X7WGFCL/p1704243108737559

Revert the affected line to use the previous literal string rather than the AppKeys class that does not yet exist on our branch.

The recently cherry-picked CVE-2023-51663 fix included refactored
code dependent on recent upstream code that was not cherry-picked.
Revert the affected line to use the previous literal string rather
than the AppKeys class that does not yet exist on our branch.
@jmarshall jmarshall requested a review from illusional January 3, 2024 03:28
@illusional illusional merged commit 0937153 into main Jan 3, 2024
5 checks passed
@illusional illusional deleted the fix-auth branch January 3, 2024 03:41
jmarshall added a commit that referenced this pull request Jan 8, 2024
PR #325 removed AppKeys.FLOW_CLIENT here as we did not cherry-pick
the rest of the AppKeys change. We later applied PR #324 which fully
merged upstream's AppKeys changes. Unfortunately when we merged that
PR via GitHub it chose #325's version of this line. :doh:
jmarshall added a commit that referenced this pull request Jan 8, 2024
PR #325 removed AppKeys.FLOW_CLIENT here as we did not cherry-pick
the rest of the AppKeys change. We later applied PR #324 which fully
merged upstream's AppKeys changes. Unfortunately when we merged that
PR via GitHub it chose #325's version of this line. :doh:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants