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

Maybe rate limit changeset size? #4805

Closed
matkoniecz opened this issue May 17, 2024 · 27 comments
Closed

Maybe rate limit changeset size? #4805

matkoniecz opened this issue May 17, 2024 · 27 comments

Comments

@matkoniecz
Copy link
Contributor

matkoniecz commented May 17, 2024

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

@tomhughes
Copy link
Member

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.

@mmd-osm
Copy link
Contributor

mmd-osm commented May 17, 2024

At least including the previous changesets‘ bounding box extents in the current database function should be doable without touching CGImap.

@tomhughes
Copy link
Member

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.

@mmd-osm
Copy link
Contributor

mmd-osm commented May 17, 2024

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.

@matkoniecz
Copy link
Contributor Author

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.

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)

@SomeoneElseOSM
Copy link

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.

@mmd-osm
Copy link
Contributor

mmd-osm commented May 18, 2024

"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.

@matkoniecz
Copy link
Contributor Author

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.

@matkoniecz matkoniecz closed this as not planned Won't fix, can't repro, duplicate, stale May 18, 2024
@yvecai
Copy link

yvecai commented Jun 16, 2024

"After the event" monitoring seems to be quite efficient except that it took quite some time for rendering servers to apply the reverts.

... 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.

@deevroman
Copy link
Contributor

deevroman commented Jun 18, 2024

Of course then they will just switch to uploading multiple smaller batches into one changeset.

But what if we sum up the area of the last edits? It seems that it will be enough to add after line 49

max_changes := GREATEST(min_changes_per_hour, LEAST(max_changes_per_hour, max_changes));
END IF;
SELECT COALESCE(SUM(changesets.num_changes), 0) INTO STRICT recent_changes FROM changesets WHERE changesets.user_id = api_rate_limit.user_id AND changesets.created_at >= CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - '1 hour'::interval;
RETURN max_changes - recent_changes;

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.

@mmd-osm
Copy link
Contributor

mmd-osm commented Jun 19, 2024

@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

@matkoniecz
Copy link
Contributor Author

matkoniecz commented Jun 19, 2024

edited, sorry for a confusion

and thanks for pointing out that I was misleading

@mmd-osm
Copy link
Contributor

mmd-osm commented Jun 21, 2024

@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.

@matkoniecz
Copy link
Contributor Author

done now

@mmd-osm
Copy link
Contributor

mmd-osm commented Jun 22, 2024

@matkoniecz : bbox size limit is in production now!

@stoecker
Copy link

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".

@matkoniecz
Copy link
Contributor Author

matkoniecz commented Jun 28, 2024

Code seems to be in zerebubuth/openstreetmap-cgimap#413 and #4908 (and maybe subsequent edits in relevant files)

We're getting the first bug report for that new "feature".

not sure why you put scare quotes there and even harder to say anything specific without link to bug report

@stoecker
Copy link

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.

@matkoniecz
Copy link
Contributor Author

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.

So pointing me at the stuff I already looked through is less then helpful.

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.

@tomhughes
Copy link
Member

The main point is that any account that has been editing for more than 28 days shouldn't see any new limits at all.

@stoecker
Copy link

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.

@mmd-osm
Copy link
Contributor

mmd-osm commented Jun 28, 2024

Assuming @b1tw153 is same account that has created https://josm.openstreetmap.de/ticket/23760 I would be fairly confident it’s not a vandal.

@b1tw153
Copy link

b1tw153 commented Jun 28, 2024

Assuming @b1tw153 is same account that has created https://josm.openstreetmap.de/ticket/23760

Yeah, that's me. I had discussed the JOSM behavior in this particular case with @matkoniecz on OSM US Slack.

@mmd-osm
Copy link
Contributor

mmd-osm commented Jun 28, 2024

By the way, we’ve been discussing the exact same issue last week already: zerebubuth/openstreetmap-cgimap#412 (comment)

@tomhughes
Copy link
Member

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.

@b1tw153
Copy link

b1tw153 commented Jun 28, 2024

That JOSM ticket says you're using the dev api? How old is the account there?

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.

@tomhughes
Copy link
Member

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.

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

No branches or pull requests

8 participants