Modify

Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#15693 closed defect (fixed)

[PATCH] Prevent double URL decoding in AddTagsDialog

Reported by: skorbut Owned by: skorbut
Priority: normal Milestone: 17.12
Component: Core remotecontrol Version:
Keywords: Cc:

Description

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

Attachments (1)

no_decode.patch (1.5 KB) - added by skorbut 6 months ago.

Download all attachments as: .zip

Change History (7)

Changed 6 months ago by skorbut

Attachment: no_decode.patch added

comment:1 Changed 6 months ago by stoecker

Owner: changed from team to skorbut
Status: newneedinfo

While the problems seems valid the fix looks wrong.

comment:2 Changed 6 months ago by 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?

comment:3 Changed 6 months ago by 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.

comment:4 Changed 6 months ago by 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?

comment:5 Changed 6 months ago by stoecker

Resolution: fixed
Status: needinfoclosed

In 13253/josm:

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

comment:6 Changed 6 months ago by stoecker

Milestone: 17.12

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain skorbut.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.