Opened 10 years ago

Closed 10 years ago

#8148 closed defect (fixed)

too large confirmation dialog displayed, and non-validated command before handling a received RemoteControl request (patch proposed)

Reported by: verdy_p Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: GUI RemoteControlHandler Cc:


When receiving a remote control from an external link presented on a web site, that orders JOSM to load a list of objects, the list of objects to load from the DB is formated in the remote command in a "compact" format like "r12345,r56789,w0123".

However such list can be quite long, and when JOSM displays its confirmation dialog, this long string contains no space and no newlines. So the dialog that appears, and that attempt to display this long list of objects, will frequently be oversized so much that even the OK/Cancel buttons won't even fit on the screen :

The dialog will just align to the right side of the screen, and most of the message will be out of screen, the dialog window exceeding its size. The only way to use the GUI of this dialog, to read the message and press one of the action buttons requires us to drag and move the very large window horizontally.

Additionally, on PCs that may have multiple displays, the parts of the window that exceeds the current display will attempt to appear on other screens placed beside the main screen ; as these screens may have distinct resolutions, and JOSM is unable to adapt the rendering of those parts using the correct resolution (for sizing fonts, or for correctly rendering text with ClearType whilealso matching the effective physical resolution of the display), this causes the FULL Windows desktop to change into a color compatibility mode, and to disable the Aero desktop style (meaning that the support of transparent or half-transparent windows is also disabled), and with a simpler color model (this affects ALL currently running applications, and cannot be stopped until JOSM is quitted completely).

Some desktop window managers (e.g. in Linux) will refuse to position or move the dialog horizontally outside the maximum left margin of the combined display (but still they won't automatically constrain these dialogs to fit completely in the display, by automatically scrollbars around the client area of the dialog window). In that case it will be impossible to even see the full message or to reach the two OK and Cancel buttons that "appear" outside of the display, very far away to the right.

For this reason, you SHOULD not size this dialog automatically by assuming that the text will fit on the screen (it does not when you just include blindly the remote control parameter string, without any reformating).

Instead, :

  • you should first reformat this list of objects to display in the remote control conformation dialog by substituting the regexp /, */ by ", ", so that linebreaks will be possible.
  • you should allow a vertical scrollbar to appear automatically in the text area showing the formatted message in the dialog above the two action buttons, instead of just using a static text area just constained on the extent of the client area of the dialog window : if this message cannot fit in a reasonable sized dialog that can be fully displayed on the desktop screen, the message will still be fully readable by just scrolling it vertically (either wit hthe scrollbar, or by moving a focus caret (or selection) in the text using the keyboard or mouse within the text area.
  • Note that the text area showing any message with some variable/substituted/inserted parts in any confirmation dialog (or any other error, exception, or progress/status/information dialogs) SHOULD ALWAYS be focusable with keyboard or mouse, in order to allow copies into the clipboard, even if it is not editable and the only action possible is to click a OK and/or Cancel button (or pressing Enter or Escape keys), even if this message is short enough to be fully visible without scrolling it.

Attachments (0)

Change History (19)

comment:1 Changed 10 years ago by verdy_p

Note that the way such extremely large moveable dialogs are rendered in the application means that it also internally allocates a very large bitmap (where the rendering will occur).

This means that lots of memory gets allocated just for this rendering bitmap.

It may also be the reason why Windows reduces the color depth or the support of transparency of windows displayed on the desktop, by disabling the Aero style, just because it could also crash some accelerated display drivers (that have internal limits on the maximum width of a bitmap for which it supports an accelerated display).

In other words : never permit any window to become arbitrarily large, always check what you want to display in them (notably each time it contains external parts for messages for which you don't know how long it could be), and allow scrollbars wherever this could exceed reasonable display sizes.

comment:2 Changed 10 years ago by verdy_p

Note that many hardware display accelerators do not support the accelerated display of bitmaps that are larger than about 16K pixels (in 32-bit color depths).

This means that any attempt to do display such bitmaps in an application that largely exceeds the maximum desktop disply size, will force Windows to use a non-accelerated rendering by software : this is what forces the whole desktop to have Aero support disabled (as it is only supported if hardware acceleration works and is enabled, notably for rendering bitmaps or filling rectangles with uniform colors).

This checking of the maximum bitmap size is currently NOT performed by the Java GUI components (they can submit to the underlying display engine for accelerated display some bitmaps that exceed the hardware acceleration limits). I think it is an implementation bug in the current Java GUI components (at least for its DirectX implementation on Windows).

comment:3 Changed 10 years ago by verdy_p

Note that the use of prerendered bitmaps is dependant on the type of GUI component you use when creating the dialog window. It is possible to avoid the allocation and rendering in this intermediate bitmap, if the GUI component can redraw itself within the visible area of the window (as instructed by the window manager which sends the appropriate info about the rectangle to redraw). It will even be faster for such dialogs whose rendering is not complex and easily accelerated by rendering it directly in the visible area of the composition window.

comment:4 Changed 10 years ago by verdy_p

The problem is located there:

in the final public void checkPermission() method that requests permission to the user using this code:

  119	        /* Does the user want to confirm everything?
  120	         * If yes, display specific confirmation message.
  121	         */
  122	        if (Main.pref.getBoolean(globalConfirmationKey, globalConfirmationDefault)) {
  123	            if (JOptionPane.showConfirmDialog(Main.parent,
  124	                "<html>" + getPermissionMessage() +
  125	                "<br>" + tr("Do you want to allow this?"),
  126	                tr("Confirm Remote Control action"),
  127	                JOptionPane.YES_NO_OPTION) != JOptionPane.YES_OPTION) {
  128	                    String err = MessageFormat.format("RemoteControl: ''{0}'' forbidden by user''s choice", myCommand);
  129	                    throw new RequestHandlerForbiddenException(err);
  130	            }
  131	        }

The problem here is that the JOpenPane.showConfirmDialog() method used (from Swing), unfortunately does not set any limit for autosizing the dialog it creates from the provided HTML message. Fixing the maximum width would require recreating an alternate dialog class checking the dimensions of the desktop, and creating the its HTML pane as a scrollable and focusable area with scrollbars.

Also the message retrieved from the abstract getPermissionMessage() method (and actually implemented in the derived RequestHandlers) is insufficiently formatted. These request handlers, registered in
or in plugins are responsible of formatting this message.

The RequesHandlers for download requests sent to JOSM via remote control, are the public static "command" members in concrete subclasses of the same package

  • LoadAndZoomHandler (also the public static member "command2")
  • LoadObjectHandler

These concrete subclasses generally implement the getPermissionMessage() method by just returning a static message, non dependant of the parameters found in the remote control command. However it is not the case in :
which contains :

40	    public String getPermissionMessage()
41	    {
42	        return tr("Remote Control has been asked to load data from the API.") +
43	                "<br>" + tr("Request details: {0}", request);
44	    }

In other words, every details of the (unparsed) request parameters are including blindly in the message to display to the user in the confirmation dialog, without any reformating for correct visual display.

I would at least change it this line 43:

43 -	                "<br>" + tr("Request details: {0}", request);
43 +	                "<br>" + tr("Request details: {0}", request.replaceAll(",\\s*", ", "));

But actually for security it is expected that this unparsed request has first been validated first to contain only valid parameters.

Another problem appears here: the remote control's RequestHandler model does not actually completely validate the requests before asking for permission to the user (if it is ever asked to, because this confirmation can be also authorized or forbidden automatically by user preferences consisting only is simple checkboxes), and then handling the requested command.

The only basic validation performed is the name of the supported command, and the presence of some mandatory parameters, but each RequestHandler subclass cannot augment the type of parameters they can validate by themselves (it is missing an abstract or overridable method to perform custom validation per command type.

As the full request is displayed here in the dialog (blindly inserted in an HTML form, ignoring the possible presence of HTML tags encoded in the command sent with the remote request) this could also be opening a security attack against a running instance of JOSM). So this militates for the addition of a prior validation of the request parameters, so that the request handler can parse the request completely and decide which parameters are more restricted or forbidden or inconrrectly formatted, or are in acceptable limits. This validation can of course save in the handler's instance itself the result of this custom parsing, before returning the control to the RemoteControl server that will ask the user if needed.

Such preliminary parsing and validation of the request is similar to what is currently performed in the LoadObjectHandler class, except that is will occur in a new validateRequest() method, instead of the existing handleRequest() method (which is called after checking authorization settings for the command, and after the interactive user confirmation).

Then the getPermissionMessage() method of the RequestHandler can be called by the RemoteControlServer : this existing method will format a message using the preparsed parameters (already saved in its own instance, ignoring the content of the original, possibly unsage full request), in a way suitable for display to the user in the HTML pane of the confirmation dialog.

As the parameters are already parsed in a directly processable list of OSM items (object type + object id), the line 43 above should then not have to perform regexp substitutions from the original request, but should build a suitable HTML list from the preparsed request parameters.

comment:5 Changed 10 years ago by verdy_p

Keywords: GUI RemoteControlHandler added
Summary: incorrectly sized confirmation dialog when receiveing a remote controltoo large confirmation dialog displayed, and non-validated command before handling a received RemoteControlRequest (patch proposed)

comment:6 Changed 10 years ago by verdy_p

Summary: too large confirmation dialog displayed, and non-validated command before handling a received RemoteControlRequest (patch proposed)too large confirmation dialog displayed, and non-validated command before handling a received RemoteControl request (patch proposed)

comment:7 Changed 10 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 5680/josm:

fix #8148 - Improve remote control handlers (requests validation, display of confirmation messages, robustness, javadoc)

comment:8 Changed 10 years ago by verdy_p

Not fixed !
Your just changed the code from :

    public String getPermissionMessage() {
        return tr("Remote Control has been asked to import data from the following URL:")
                + "<br>" + request;


    public String getPermissionMessage() {
        return tr("Remote Control has been asked to import data from the following URL:")
                + "<br>" + args.get("url");

which means that you still display the URL as is, which may still be very long and could still contain some HTML, even if the URL passed the URL validation test.

The rest of your changes was just to validate the URL parameters. But the case for which I wantred a patch was not about malicious requests, but about perfectly valid request URLs.

As the dialog container used to render this text will still be the default Swing alert dialog, whose size is not constrained and which is not scrollable, the rendered dialog will still overflow the maximum display width, without using any scrollbar, and if there's NO spaces in this URL, it will cause the dialog to be still much too large, causing Java to turn the display in unaccelerated compatibility mode (disabling Area for example, i.e. switching with Windows switching from the normal driver to the default VESA driver : other applications will immediately hang or won't be visible on the same desktop).

Try passing a long request URL without any space, notably an URL containing a long list of object IDs (these IDs are sent in the URL just as a comma-separated list with one letter 'n', 'w', or 'r' immediately followed by the numeric ID in many cases, but as yu did not transform the URL to include at least a space after each comma, the dialog will be autosized with an extremely large width...)

Do you intend to replace the default Swing alert dialog by a smarter dialog featuring automatic scrollbars when needed ?

The bug request above was explicit about why the change was needed, it should be now:

    public String getPermissionMessage() {
        return tr("Remote Control has been asked to import data from the following URL:")
                + "<br>" + args.get("url").replaceAll(",\\s*", ", ");

to solve the problem with common requests passing long lists of object ids ; it does not matter here if the URL displayed in the confirmation dialog is transformed a bit, anyway this is still a technical display, and given that you have already parsed the arguments, you could generate here a smart list of object IDs and arguments, formatted as HTML, directly from args : write a args.formatHTML() function to make the dialog more friendly, this HTML may even include a scrollable box, like in :

    public String getPermissionMessage() {
        return tr("Remote Control has been asked to import data from the following URL:")
                + "<br><textarea style='max-width:80%;max-height:50%;' readonly='readonly'>"
                + args.get("url") // note that this URL may contain unsafe HTML (including scripts)
                  .replaceAll("&", "&amp;")
                  .replaceAll("<", "&lt;")
                  .replaceAll(">", "&gt;")
                + "</textarea>";

Note however that I did not test if the HTML renderer (of Java Swing dialogs) supports CSS styles for max-width and max-height in Java 6 ; if not, youmay need to use rows='10' cols='72' instead or use your own dialog constructor.

comment:9 Changed 10 years ago by Don-vip

Could you please give me an example of website using the 'import' command with a long list of object IDs ? The majority of live examples I found are using the 'load_and_zoom' and 'load_object' commands for loading specific primitives.

Concerning the risks you are describing with malicious HTML code, unless you give me examples making real trouble (like a crash or data corruption, and not just a weird-looking dialog box), I don't consider it is worth the trouble right now. There are a lot of other tickets to focus on.

comment:10 Changed 10 years ago by anonymous

For example:

Click on the link "Charger toutes les relations : * basé sur GEOFLA 2011" at the bottom of the page.

(it tries to load in JOSM the long list of all commune relations in the department, this means a long list of comma-separated "r<id>" within a single parameter).

It performs the follwing HTTP request via the remote control HTTP server running in JOSM:

The conformation dialog exhaust the screen size, it forces Java to change its display interface to compatibility mode (the rendered dialog width cannot be hardware-accelerated as it is larger than what display acceletators support, which is almost never more than 2048 pixel wide), so immediately the Aero look on Windows 7 is turned off (and it will not resume when the dialog is closed).

The OK or Cancel buttons are not even visible on screen ! You need to slide the window manually to the left over a long distance to find them (or you have to "guess" that you can press "Enter" or "Escape" keys, or Alt+F4 to dismiss the dialog, but the normal desktop display will never be resumed, as long as the Java VM is running, you have to quit JOSM completely; during this time, any other processes on the desktop will no longer be accelerated (for example this can cause the web browser to hang, or to stop playing ongoing videos, but generally this causes a crash in Flash components loaded in those browsers).

Additional note: it seems that recently this link at the bottom of report pages on seem to no longer generate the list (this list is empty now, most probably because it caused JOSM to bug in its display), so for now only a single object is now editable from the displayed list. But there was effectively a long list of ids there.

I will look at other examples existing on the web. But it is clear that any website could build an arbitrarily long list of parameters in its GET URL, which would still be valid.

The remote service should also not depend on the fact that parameters are passed in a GET query string or in a POST data. It should parse those request parameters independantly of the GET/POST method used, then present in the dialog the URL without any query string, and *separately* the parsed and *validated* arguments. When accepted parameters are recognized (like here a list of objects, this list should also be parsed, and stored internally, so that these objects *may* be presented in the confirmation dialog (or the dialog could just display a more synthetic view of these possibly long lists, by just giving how many items are in this validated list).

The confirmation dialog should also indicate if there are non-recognized parameters, and may be list their names and values (as long as they are NOT too long, and IF they are designed to not cause some HTML or script to be injected : I gave the code above to allow this, which just consists in replacing all "&", "<" and ">" characters ; you could also limit the length of strings displayed by truncating it, and replacing the rest of the parameter name or value with "<em style='background:#EEE;border:1px solid #AAA;font-size:smaller;line-height:normal'>truncated</em>" on display, to make sure that the confirmation dialog will indicate to the user that this is a very long parameter name or very long parameter value). Ideally, the list of arguments should be formated as a list (e.g. an HTML <li></li> or an HTML <table> with two columns for the parameter name and the parameter value ; if a value was parsed as being a list, there should be an action button to detail it, but this list should be hidden by default, with a tringular button to open/mask this list).

Ideally you should also NOT use the default Java Swing alert, it is not safe (even if it requires no other Java code). You should have another dialog constructor featuring the needing scrollbar automatically : as it displays its message by interpreting as an HTML document, the dialog should contain a browser window object, which behaves like traditional browers for long documents which are implicilty auto-scrollable; this way the dialog will remain at reasonnable size and will always fit on screen ; the dialog window may also be resized by the user as he wants ; the parent of this dialog should also be the parent window of JOSM, not the default desktop as it is now in the default Swing alerts).

comment:11 Changed 10 years ago by verdy_p

Resolution: fixed
Status: closedreopened

So, because you have actually NOT corrected what was the MAIN request of this bug, I reopen it. What you have changed in JOSM is COMPLETELY unrelated to this bug.

comment:12 Changed 10 years ago by Don-vip

No need to be rude. With such a tone, don't be surprised if bugs reported by polite people are fixed before yours.

comment:13 Changed 10 years ago by anonymous

I was not rude, it's just that you actually did not change the status of this initial bug but corrected something else.
Just try it yourself on this page :
click on the bottom link to load the communes, it will send this long request:

RemoteControl::Accepting connections on port 8111
RemoteControl received: GET /import?url=,147124,145704,148821,146573,145615,146765,146651,75651,23784,145639,147346,21557,146571,139139,148030,146706,145686,147866,138967,139526,148903,139510,149315,142161,138994,145703,36209,149288,148542,146650,30232,147348,146773,139251,147858,139161,146766,140255,149565,145494,147999,140029,146708,149566,138979,142544,145614,140253,149269,145571,138962,145870,147869,142223,146597,149289,140522,146643,140123,146918,142153,145505,142146,139261,140326,103775,145588,145568,147996,142155,140514,148006,145600,140032,145640,145099,140129,145112,30261,149287,145678,145587,140530,145677,142241,139111,139525,140126,147865,146660,147335,146682,142222,138965,139595,139873,148033,142551,146758,139003,142144,146609,146757,142672,140523,140752,139598,146779,146572,75655,145530,145586,147872,140024,139112,139617,139200,139129,145685,139530,146782,139570,139262,140026,75650,148926,139596,146776,146688,139006,139174,148362,142237,138995,21591,145679,140252,145499,30233,139202,139176,145688,147336,149049,140760,139173,140088,146672,138993,145101,145674,140130,142160,140328,36208,139255,139156,140031,138961,149464,140085,139571,139535,26640,146607,142221,140516,145705,147339,145496,140330,145495,146626,145572,140030,145866,145681,140086,139154,142150,147990,145527,145123,146722,140028,140529,142151,145531,145673,103638,142158,139516,142149,138974,148032,140761,145616,146623,147327,146685,145684,146614,145529,146669,142152,139199,138972,139162,139007,146625,146666,145124,139172,26637,147321,146611,139874,148914,145528,145585,147338,140527,140023,73946,147352,149480,145687,140754,140517,146648,140518,145098,140121,148025,148014,147333,145113,140127,26657,139201,142506,145864,140520,146608,142242,142226,30300,147270,147230,145641,145680,146705,139875,145115,21541,139153,147323,146588,140256,140757,149442,140025,149463,140759,145868,142156,139001,147350,30242,145589,140525,139614,146662,142227,145500,138958,140022,140755,140763,139250,146570,148017,139253,139886,145597,146653,140327,145702,145097,148902,140526,148028,138959,148363,142154,146917,145638,142220,139511,148029,140120,142159,140528,139130,145874,148009,146582,146664,147351,139597,140124,148008,139254,147980,140131,147995,148917,142236,138966,147975,139618,139569,142219,140331,146628,145126,140254,149265,147978,145599,138977,139615,149268,138980,140125,140329,296220,147982,146703,140756,142157,139529,149445,147221,147986,139133,145125,145617,142225,145675,140128,146719,139252,139132,145100,149322,139163,139505,145111,146711,139260,145671,139568,146647,139616,139140,146649,140521,140758,138973,75654,140084,145618,142555,146715,140762,145601,147873,147868,148026,147343,149479,140087,140753,146751,142148,148000,149497,142224,139524,148010,142162,140122,145570,146767,139175,139138,145672,139515,139155,149229,139164,145114,147867,145620,146707,139171,145569,140257,145493,145598,147987,148910,146696,147328,146574,146668,140027 HTTP/1.1

comment:14 Changed 10 years ago by verdy_p

And note that if the parameters contain some HTML tags, they will be rendered as HTML (in a limited way because Java Swing usues for this dialog a JTextComponent (instantiated from a JLabel); as your message already starts by prepending "<html>", it will render these HTML4 inline elements :
<b>...</b>, <i>...</i>, <u>...<u>, <s>...</s>, <var>...</var>, <em>...</em>, <font>...</font>, <small>...</small>, <big>...</big>
as well as these object elements:
<br>, <image>, <input>, <textarea>...</textarea>
as well as these HTML4 block elements :
<p>, <ul>, <ol>, <li>, <dl>, <dd>, <dt>, <table>,<caption>,<thead>,<tbody>,<tr>,<th>,<td>
Some CSS is also supported (font, color, background), but not many essential CSS properties (width, height, text-align, ... border is bugged for tables)

Scripts events are normally ignored and inactive, unless there are registered event handlers in the dialog (but I don't think that they will run the javascript, they would just post an event to the registered callback, if there's one).

Anyway the displayed confirmation dialog is still unusable when you display the full long URL

comment:15 Changed 10 years ago by verdy_p

Note also that the confimation dialog does not display the query if query parameters ares sent with a POST method (encoded within a form), but it will attempt to render the full URL including with their non-validated extra arguments.
The way it is currenlty displayed, it is also too technical (such display is fine in the Java console, but not in the application in the confirmation dialog, which should be showing something more useful. Note that you have already parsed and validated the query, so the dialog should display the parsed parameters, not the full URL.

comment:16 Changed 10 years ago by verdy_p

For another even longer query, look at the query sent from there:
(and it is still not complete, as there are missing communes in this department)

RemoteControl received:

Note also that if the query is accepted, all these relations will be loaded, but only one of them will have its member ways loaded, all others are actually loading only their member nodes (in fact here only the member with "admin_centre" role). This element is apparently randomly chosen (random choice? from the randomized order of the Collection containing the parsed arguments?)

The Java console also displays this, while loading the relations:

L’élément 'note' trouvé dans le flux d’entrée n’est pas défini. Abandon.
L’élément 'meta' trouvé dans le flux d’entrée n’est pas défini. Abandon.

(which means that some objects are abandonned during their download due to the presence of some note or meta tags... I wonder why, this is probably another bug)

comment:17 Changed 10 years ago by Don-vip

In 5691/josm:

see #8148 - Remote control: URL validation in import handler + find suitable download tasks + dynamic width of confirmation dialog box

comment:18 Changed 10 years ago by Don-vip

Speaking in uppercase is generally considered as rude, sorry. But thanks for being constructive by giving your example.

My first changeset did correct the problem in some way for load_and_zoom and load_object (even if I admit the message can be improved). You never mentioned that you were talking of the import handler (you were just referring to "a remote control from an external link presented on a web site, that orders JOSM to load a list of objects"). OSM Primitives are generally loaded with these two handlers.

Here, the import handler is used because the OSM-FR website does not make queries to the main OSM API but to the OSM-FR API. It's kind of a hack as the import was designed to load an .osm file and not make queries to the API, but that's ok, I took advantage of this ticket to improve this handler as well (it now relies on the same mechanisms used in "Open Location" command).

I will do nothing concerning your other remarks (presence of HTML tags, POST request and so on) as I don't see them as real problems. And you said it yourself, this issue does only concern the width of the confirmation dialog.

So please close this ticket tomorrow with latest JOSM if the confirmation dialog fits on your screen.

comment:19 Changed 10 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

Modify Ticket

Change Properties
Set your email in Preferences
as closed The owner will remain team.
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.