-
Notifications
You must be signed in to change notification settings - Fork 936
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
Maybe rate limit changeset size? #4805
Comments
I don't think this is something we could make a decision on - it should really be DWG to decide if they need this and if so what the limits should be and then cgimap would be the primary place that needs changing rather than this code. |
At least including the previous changesets‘ bounding box extents in the current database function should be doable without touching CGImap. |
How does that help? We want to block the upload if the bounding box is too large surely otherwise at least one changeset will get through. Of course then they will just switch to uploading multiple smaller batches into one changeset. |
Yes, at the moment we would have to let one large changeset (it’s really the first upload) go through and could block subsequent ones. I believe this would already improve the current situation. Blocking the first large bbox upload requires changes to CGImap. I already thought of passing the current bounding box and user id to some db function and block the upload depending on the result. This shouldn’t be too difficult to do. |
Even that would be a big help! Limiting them to one changeset would force vandal to create more accounts, and even putting multiple changes in one edit makes reverts easier. Though ideally even first one would be blocked (obviously, it will not block all vandalism but will make some types of attacks a bit harder) |
To add some numbers here, this account managed 23 changesets, but they were submitted in just 3 minutes. The DWG currently does "after the event" monitoring and potentially blocking (which caught this user at the end of their editing spree), and could potentially also look at bounding box size too, but API-level blocking is a whole other issue - both I presume to implement in the first place and to make sure that "innocent" users are not unduly affected. I can probably figure out how to get a list of accounts who might have been affected by a block on "large" changesets among the first X submitted (to see which were actually genuine), if it would help. |
"After the event" monitoring seems to be quite efficient except that it took quite some time for rendering servers to apply the reverts. I would probably try to focus more on improving this part, rather than introducing more API limits right now. We've introduced upload rate limits before, but it caused a lot of backlash from some parts of the community. The most challenging part is to work out suitable criteria to minimize side effects. Implementing the changes is pretty easy in comparison. |
I will close it as it does not seem to be an obviously good idea, but if anyone who knows more than me things otherwise feel free to reopen. Or open new issue with some analysis about whether any (and how many) useful edits would be affected. |
... except that troubles exists for a vast variety of infrastructures using Openstreetmap diffs to keep up to date. What about apps updating their maps once a month ? Blocking changesets is the only upstream action that would help all Openstreetmap data users, however small or big they are. Openstreetmap.org raster tile CDN is certainly important, but by far not the only service affected. |
But what if we sum up the area of the last edits? It seems that it will be enough to add after line 49 openstreetmap-website/lib/database_functions.rb Lines 49 to 54 in 2d09b94
something like SELECT COALESCE(SUM((changesets.max_lon / 10000000.0 - changesets.min_lon / 10000000.0) +
(changesets.max_lat / 10000000.0 - changesets.min_lat / 10000000.0)), 0.0)
INTO recent_changesets_area
FROM (select *
from changesets
where changesets.user_id = api_rate_limit.user_id
and changesets.created_at >= CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - '5 minute'::interval
order by changesets.created_at desc
offset 1) changesets;
max_changes := max_changes / GREATEST(CEIL(recent_changesets_area), 1); This will skip the first big edit, but it will give DWG and the community time to react to big edits. |
@matkoniecz your comment in https://community.openstreetmap.org/t/the-osm-standard-tile-layer-looks-wrong-white-lines-abusive-comments-etc/111583/468 is not quite accurate, we still need to integrate this logic on the CGImap side. Nevertheless the major part of the work is done now. Kindly ask you to update your comment on the forum. Thanks! -> CGImap follow up in zerebubuth/openstreetmap-cgimap#412 |
edited, sorry for a confusion and thanks for pointing out that I was misleading |
@matkoniecz : you can also encourage people to do some testing on https://master.apis.dev.openstreetmap.org now. Code is ready for testing. They need to create a new account, though, and then can experience first hand, how it feels to be a newbee with limited bbox size allowance only. |
done now |
@matkoniecz : bbox size limit is in production now! |
What are the exact details of the "bbox size limit is in production now!". We're getting the first bug report for that new "feature". |
Code seems to be in zerebubuth/openstreetmap-cgimap#413 and #4908 (and maybe subsequent edits in relevant files)
not sure why you put scare quotes there and even harder to say anything specific without link to bug report |
Before asking I read through related tickets, pull requests and some code which I don't know en detail and searched for an official announcement and then I decided to ask because all that search didn't help me to find the finally implemented details. So pointing me at the stuff I already looked through is less then helpful. |
I am not aware of any official announcement with detailed code explanation or guide how to avoid being blocked. Overall new accounts can get their edits refused with error 413 if bounding box is big. If there are specific cases of edits being blocked that should not be, knowing details or any specific info at all would be good first step toward potential changes.
Maybe I missed it but your comment had nothing that would allow to know that you already looked at these, so I linked as there was chance it would be useful. |
The main point is that any account that has been editing for more than 28 days shouldn't see any new limits at all. |
Thanks. That's a very important fact and something I hoped. That means the ticket for JOSM is very likely from one of the vandals and thus priority is very low. |
Assuming @b1tw153 is same account that has created https://josm.openstreetmap.de/ticket/23760 I would be fairly confident it’s not a vandal. |
Yeah, that's me. I had discussed the JOSM behavior in this particular case with @matkoniecz on OSM US Slack. |
By the way, we’ve been discussing the exact same issue last week already: zerebubuth/openstreetmap-cgimap#412 (comment) |
That JOSM ticket says you're using the dev api? How old is the account there? I can't actually see and 413 error in the log for the dev instance in the last couple or days, or any from JOSM on the production servers. |
I guess the account and the activity on the dev api was from seven days ago. I tried to reproduce the JOSM behavior this morning but didn't get a 413 error from the dev api today. |
Right so I can see that and the account was less than 24 hours old at that point so yes it will have had limits and after a week those will have increased and they will increase further until after 28 days the limit reaches the full size of the world. |
implemented in #4908
Problem
It is easy to make malicious or accidental edit that involves dragging node across the world.
Description
In some rare cases it is preferable to make edit with huge changeset size,
But this kind of edits are (1) rare (2) rarely done by new mappers.
While many vandals do them as a new accounts.
Maybe blocking edits with huge changeset size for new mappers, or causing them to quicker exhaust edit quota would be useful and doable?
Main problem is that it may block new mappers from say adding language label to USA/Russia. And obviously, it would take effort to implement/maintain such new code.
Some of previously applied restrictions for new user accounts: #4196 #3135 #2342 #1543
Screenshots
No response
The text was updated successfully, but these errors were encountered: