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

What is a reason for querying extreme-ip-lookup.com on client side? + (is api.ipify.org required too?) #159

Closed
phpony opened this issue Apr 17, 2023 · 5 comments
Labels
improvements Improvements

Comments

@phpony
Copy link

phpony commented Apr 17, 2023

Hello!

Found out that client.js is making the request to https://extreme-ip-lookup.com/json/ when you join the room:

There's the config option:

const peerLoockupUrl = 'https://extreme-ip-lookup.com/json/?key=demo2'; // get your API Key at https://extreme-ip-lookup.com
const peerIPUrl = 'https://api.ipify.org?format=json';

There's the function using it:

/**
* Get approximative peer geolocation
* Get your API Key at https://extreme-ip-lookup.com
*/
async function getPeerGeoLocation() {
console.log('03.1 Get peer geo location');
fetch(peerLoockupUrl)
.then((res) => res.json())
.then((outJson) => {
peerGeo = outJson;
})
.catch((err) => console.warn(err));
}

Here we pass peerGeo obtained from 3rd-party to sever in connection payload:

mirotalk/public/js/client.js

Lines 1442 to 1444 in b7cd692

peer_info: peerInfo,
peer_geo: peerGeo,
peer_ip: peerGeo.query || myWanIP,

Here we completely ignore it:

mirotalk/app/src/server.js

Lines 650 to 670 in f2d70ee

socket.on('join', async (cfg) => {
// Prevent XSS injection
const config = checkXSS(cfg);
// log.debug('Join room', config);
log.debug('[' + socket.id + '] join ', config);
const {
channel,
channel_password,
peer_ip,
peer_uuid,
peer_name,
peer_video,
peer_audio,
peer_video_status,
peer_audio_status,
peer_screen_status,
peer_hand_status,
peer_rec_status,
peer_privacy_status,
} = config;

By default if you do not provide any API key extreme-ip-lookup.com is returning dummy data. My uBlock is blocking this host and this doesn't break anything. Also because it's not used anywhere on server side, I've removed this peerGeo from payload completely on my local instance without any problems. The IP is still provided by api.ipify.org from myWanIP variable.

Seems like querying extreme-ip-lookup.com is either some old obsolete code or something planned for future versions? And even if it will be used in future - I don't think the sever in any form should trust any peerGeo data requested from 3rd-party on client side, because it can easily be forged. It would be better to rely on geoip-lite on server side then.

So, for now it looks like we're slowing down the initial connect by waiting for 3rd-party request to extreme-ip-lookup.com for.. nothing?..

P.S. Maybe it would be better to get rid of api.ipify.org too and to just rely on remote ip on server side...

@phpony
Copy link
Author

phpony commented Apr 17, 2023

It's probably possible to just rely on something like this

peer_ip = socket.handshake.headers['x-forwarded-for'] || socket.conn.remoteAddress.split(":")[3];

at server side instead of any 3rd-party services on client side. That'll work faster and the app will not depend on other services uptime. I don't see any cases when 3rd-party services will return any other IP than code above will. Except some heavily misconfigured servers.

@phpony phpony changed the title What is a reason for querying extreme-ip-lookup.com on client side? What is a reason for querying extreme-ip-lookup.com on client side? + (is api.ipify.org required too?) Apr 17, 2023
miroslavpejic85 added a commit that referenced this issue Apr 17, 2023
@miroslavpejic85 miroslavpejic85 added the improvements Improvements label Apr 17, 2023
@miroslavpejic85
Copy link
Owner

miroslavpejic85 commented Apr 17, 2023

Hey @phpony!
extreme-ip-lookup.com was initially totally free, then it became paid (geoip-lite you mentioned, seems like a great alternative to me and agree with you, that should be handled by server). However I was using it for debugging purposes, and you're right, this data on the server side it's not necessary/used at all. As for api.ipify.org, replacing it with your server-side proposal, seems excellent to me! I removed all unnecessary/unused code.
Thank you so much!

@miroslavpejic85
Copy link
Owner

miroslavpejic85 commented Apr 18, 2023

@phpony Now I remember why I used extreme-ip-lookup on the client side :) It was a simple hack to not pay for the API key, as with the key=demo2 using it on the client side, in more cases you get the info, while if you use that key on the server side, no. Try this one: https://extreme-ip-lookup.com/json/?key=demo2
Since I've been asking about that feature in the MiroTalk forum, I was thinking of bringing it back... or using geoip-lite on server side, but got less info compared to extreme. Probably in some use cases for private project, this functionality can be useful to someone for collect statistics, not depend of google analytics and so on. In my case it was just for debugging purposes.

@miroslavpejic85
Copy link
Owner

Found the alternative: https://www.geojs.io/docs/v1/endpoints/geo/ this works on server side as well.

@miroslavpejic85
Copy link
Owner

miroslavpejic85 commented Apr 18, 2023

Make it as optional if someone need (default false), implementation on server side.

# IP lookup
# Using GeoJS to get more info about peer by IP
# Doc: https://www.geojs.io/docs/v1/endpoints/geo/

IP_LOOKUP_ENABLED=false # true or false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvements Improvements
Projects
None yet
Development

No branches or pull requests

2 participants