-
Notifications
You must be signed in to change notification settings - Fork 40
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: retrieve client IP from X-Forwarded-For
header
#428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to use axum_client_ip
instead for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we use a library to abstract this away in Verify API or somewhere else - maybe @xDarksome or @chris13524 know?
But as a hotfix this shouldn't wait for that - can improve later
You mean this? https://github.com/WalletConnect/verify-server/blob/main/src/http_server/mod.rs#L26 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the comment.
.get("X-Forwarded-For") | ||
.and_then(|header| header.to_str().ok()) | ||
.and_then(|header| header.split(',').next()) | ||
.and_then(|client_ip| client_ip.trim().parse::<IpAddr>().ok()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks scary not to handle any errors here. Malformed X-Forwarded-For
can lead to 500 errors without any debug information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't the and_then
chain default to an Error
and ultimately to a None
if anything goes wrong?
Yeah, I added it but
|
It's a hotfix and I think it should have the same end result. But we can overhaul this after. |
Description
Since the migration moved the service behind an ALB, we cannot retrieve the client IP directly for geo-location.
This leverages the
X-Forwarded-For
header injected by the LB instead.How Has This Been Tested?
Unit tests
Due Diligence