[PATCH] Prevent double URL decoding in AddTagsDialog

[skorbut]

It looks to me that tags, given as arguments using the "addtags" parameter, are URL-decoded twice.

Example: ​http://localhost:8111/load_and_zoom?select=currentselection&bottom=46.85333&top=46.853530000000006&left=9.528349&right=9.528649&addtags=contact%3Aphone%3D%2B41%2081%20254%2024%2024

The phone number starts with %2B, corresponding to +. However, the + symbol is not visible in the tag-import dialogue. My understanding is, that a call similar to Utils.decodeUrl() has happend before somewhere in the code. Therefore the + sign will be URL-decoded again, which corresponds to a space. This space however will be removed some lines further down due to the call tag1.trim().

The attached patch removes the (second) call to Utils.decodeUrl().

[stoecker]

While the problems seems valid the fix looks wrong.

[skorbut]

@stoecker: Can you be more specific? Why do you think it looks wrong? What would you like to be changed/done in another way?

[stoecker]

The comment you change is correct in the previous form. Also the fix prevents a double decoding, but actually at this point the text should not be decoded otherwise values and text could be mixed (e.g. an addtags= inside a value). The fix thus seems to be necessary somewhere earlier in the processing chain.

[skorbut]

@stoecker: You mean a case such as this

​http://localhost:8111/load_and_zoom?addtags=contact%3Aphone%3D%2B41%2081%20254%2024%2024%7Caddtags=addtags=asdf&select=currentselection&bottom=46.85333&top%3D46.853530000000006&left=9.528349&right=9.528649

?

I tested this on my patched code and it works as it should: Two tags are recognized:

1. "contact:phone" = "+41 81 254 24 24"
2. "addtags" = "addtags=asdf"

The detection of the different query parameters (select, search, addtags, changeset_comment, etc.) happens in ​https://github.com/openstreetmap/josm/blob/b4c3e3945fba7f9bf31a2c8dbbb92c4dea494d81/src/org/openstreetmap/josm/io/remotecontrol/handler/RequestHandler.java#L231-L234 This splitting happens based on the occurrence of the ampersand & in the undecoded URL. (Only) in the case of the addtags query parameter, the different tags are then further split up based on the occurrence of the pipe character | in the decoded URL (part). See either ​https://github.com/openstreetmap/josm/blob/b4c3e3945fba7f9bf31a2c8dbbb92c4dea494d81/src/org/openstreetmap/josm/io/remotecontrol/AddTagsDialog.java#L282 or https://josm.openstreetmap.de/attachment/ticket/15693/no_decode.patch (line 281). Since at this point the URL has already been decoded at least once, we can split using the pipe and don't have to use %7C. Because of that I also changed the comment, since the incoming URL in AddTagsDialog.addTags() is already decoded (with or without my patch).

I still can't see in what case my patch would fail. Can you elaborate?

[stoecker]

In 13253/josm:

fix #15693 - patch by skorbut - fix double URL decoding

[stoecker]

Ok. At that place the string is already split and decoded. Fine. So it is only the wrong comment change which confused me. I changed your comment change before checkin.

The splitting of arguments is also bad, as it first decodes and the splits by "=". As long as arguments are only alphanumeric it has the same result, but it is bad style. But I'm too lazy to test a fix ATM, so I leave it...