Catching overlarge bounding boxes to defend against bad edits

[pnorman]

From #2427

We might still want some checks in the future to catch overlarge bounding boxes to defend against bad edits that will lead to a large number of tiles to be expired, but that is a different issue.

I've been thinking about this as well but haven't yet seen discussion. I think detections will need to vary with the tagging so belong in the lua code of a transform. A 1000 square km building is an obvious bad edit but could be a valid lake.

Right now it's possible to do some detection in flex transforms and reject objects. Is there additional information that would help flex code detect bad edits? Do we need something more sophisticated than rejecting objects?

[joto]

In theory you could add a check in Lua code for every geometry using the get_bbox() function before you add it to a table to make sure it is "reasonable". But that would mean quite a lot of boiler-plate code. Another option would be to allow users to set a max bounding box on tables and do the check then behind the scenes in osm2pgsql. But that would still put the burden on the osm2pgsql user to configure this properly and chances are that not many people will do this. We could avoid large expire lists for, say, building layers this way, but other layers (like boundaries) will still need to allow huge areas, so I am not sure how useful this would be. The question is also whether we are defending against a mistake somewhere (like a mistagging or a moved node) or against an attacker.

[pnorman]

Is it bbox that matters or area?

My biggest concern is protecting performance and allowing subsequent fixes to be applied quickly. Expiring millions of tiles won't work well with most systems and will delay the updates that fix the error

[joto]

At the moment we do tile expire of polygons by bbox, so it is actually bbox, but I am working on fixing that. Hopefully soon it is the area that matters (or the length of the polygon boundary, if expiry is set to only do the boundary), or, even more exact, the number of tiles affected. But bbox is a proxy that's cheap to generate and relatively easy to understand for users.

Using "tiles" as measure is problematic because people don't necessarily have a good intuition how large tiles are in specific zoom levels. And a building that's right at a tile corner can invalidate 4 tiles but is usually much smaller than a tile. Using "area" is probably better than tiles, but it still suffers from the usual problem: Is that m² or km² or whatever and is that real area or Web Mercator units. (Although bbox also has a similar problem.)

[joto]

Thinking some more about this, I am asking myself whether this is really about the geometry itself, not just the expire tiles. Drawing a 1000km² building is already a problem even if there is no expiry involved. That would give us also more options on how to do the checking. We can do this today, calculate the area or the length of a feature in Lua. It isn't a problem to not write such a geometry to an output table in which case it would also no show up in an expire list. So if somebody changes a building and makes it 1000 times bigger than before it would simply vanish, triggering an expiry of the tiles covering the old geometry. (Actually detecting that this is a problematic change and leaving the old object in place would be better, but that's a whole other can of worms.)

[joto]

Here is an idea: We could do this check in Themepark. Allow setting a function for each table that takes a geometry and returns a geometry. It can simply return nil if it wants to reject the geometry. Themepark would call this function before doing the insert. We can provide simple stock functions that check for max length or max area and so on and provide a default function that's used if you don't set anything that will reject, say, all areas larger than 10% of the planet.