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

feat: fix open redirect vulnerability #1899

Merged
merged 14 commits into from
Jan 21, 2025
Merged

feat: fix open redirect vulnerability #1899

merged 14 commits into from
Jan 21, 2025

Conversation

Ani1357
Copy link
Contributor

@Ani1357 Ani1357 commented Jan 17, 2025

This PR fixes an open redirect vulnerability affecting logged in users. The issue was caused by the nginx config in the oauth2-proxy ingress resource.
The redirect will be allowed only for domains matching a regex expression like the one below:
^([a-zA-Z0-9-]+\.){0,2}lke317513\.akamai-apl\.net(%2F)?(/.*)?$
The lke317513\.akamai-apl\.net is taking the value of cluster.domainSuffix provided in the apl installer chart values.
You can test the expression here: https://regex101.com/r/Yk6RMh/1

Copy link
Contributor

@CasLubbers CasLubbers left a comment

Choose a reason for hiding this comment

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

It works for linode clusters with the clusterId. But does this also work if you do a manual install with your own domain? I haven't tried it but curious. Maybe we can keep it like this for Linode/managed installation but make it less strict if you use something else?

@Ani1357
Copy link
Contributor Author

Ani1357 commented Jan 20, 2025

It works for linode clusters with the clusterId. But does this also work if you do a manual install with your own domain? I haven't tried it but curious. Maybe we can keep it like this for Linode/managed installation but make it less strict if you use something else?

Good point. Updated the code so that it fits both scenarios. The regex expression is not directly constructed from the domainSuffix value provided in the apl chart values.

@Ani1357 Ani1357 requested a review from CasLubbers January 20, 2025 15:32
@Ani1357 Ani1357 requested a review from merll January 21, 2025 08:57
@j-zimnowoda j-zimnowoda self-assigned this Jan 21, 2025
@j-zimnowoda j-zimnowoda enabled auto-merge (squash) January 21, 2025 18:09
@j-zimnowoda j-zimnowoda merged commit f180cc9 into main Jan 21, 2025
5 checks passed
@j-zimnowoda j-zimnowoda deleted the APL-479 branch January 21, 2025 18:11
Ani1357 added a commit that referenced this pull request Jan 22, 2025
Co-authored-by: jeho <[email protected]>
(cherry picked from commit f180cc9)
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.

3 participants