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

Update appservice tests to check for access token in header vs query params #1362

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Jul 27, 2023

Synapse v1.81.0 deprecated application service authorization via query parameters - authorization should now be sent via Authorization header. This PR (I hope, Perl is not my native language) changes two appservice tests to stop checking the query params for the access token and to check for them instead in the Authorization header.

@H-Shay
Copy link
Contributor Author

H-Shay commented Jul 27, 2023

Seems that the Dendrite failures are not related?

@DMRobertson
Copy link
Contributor

Sanity check: the spec says

The access_token should be supplied through the Authorization header where possible to prevent the token appearing in HTTP request logs by accident.

@DMRobertson
Copy link
Contributor

Seems that the Dendrite failures are not related?

I concur, looks like Dendrite is failing on the main branch with the same set of tests.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sane, and I can see similar snippets in sytest elsewhere.

@H-Shay
Copy link
Contributor Author

H-Shay commented Aug 3, 2023

Merged #1362 into develop, thanks for the review!

@H-Shay H-Shay merged commit 961e863 into develop Aug 3, 2023
@H-Shay H-Shay deleted the shay/remove_ap_query_params branch August 3, 2023 18:43
MatMaul pushed a commit that referenced this pull request Aug 4, 2023
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