Pluto
  1. Pluto
  2. PLUTO-579

Some render parameters are lost if they contains semicolon

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.1.7
    • Fix Version/s: 2.0.4, 1.1.8, 2.1.0
    • Component/s: portal driver
    • Labels:
      None

      Description

      If one render parameter contains semicolon, those parameters after it in the url are lost
      Like the URL below
      _pm0x3system-database0x2DBWizard!1134683811|0_view/rp0x3system-database0x2DBWizard!1134683811|0_rarPath/org0x2tranql0x3tranql-connector-ra0x30x3rar/rp0x3system-database0x2DBWizard!1134683811|0_driverClass/com0x2microsoft0x2sqlserver0x2jdbc0x2SQLServerDriver/rp0x3system-database0x2DBWizard!1134683811|0_mode/params/rp0x3system-database0x2DBWizard!1134683811|0_dbtype/SQL0x8Server0x82005/rp0x3system-database0x2DBWizard!1134683811|0_adapterDisplayName/TranQL0x8Generic0x8JDBC0x8Resource0x8Adapter/rp0x3system-database0x2DBWizard!1134683811|0_urlPrototype/jdbc:sqlserver:0x30x3%7BHost%7D:%7BPort%7D;DatabaseName=%7BDatabase%7D/rp0x3system-database0x2DBWizard!1134683811|0_transactionType/LOCAL/_rp0x3system-database0x2DBWizard!1134683811|0_name/ddddd

      The parameter urlPrototype contains semicolon, while I calling renderRequest.getParameter("name"), it will return null.
      After some investigations, I found in the class PortalURLParserImpl, line 115

      String pathInfo = request.getPathInfo(); <--- the pathInfo got from the request is truncated by the semicolon.
      if (pathInfo == null) {
      Maybe, we need to add the ; to the ENCODINGS arrays ?
      Thanks !

      1. PLUTO-579-0921.patch
        6 kB
        Ivan
      2. PLUTO-579.patch
        5 kB
        Ivan

        Activity

        Ivan created issue -
        Hide
        Ivan added a comment -

        I created a patch for it, please help to review, thanks !
        The changes include :
        1. Add semicolon to the encoding array
        2. Add codes to handle those replaced strings, e.g. if the parameter is '0xaabc', make sure '0xa' is not replaced with ? when decoding the parameter value.

        Show
        Ivan added a comment - I created a patch for it, please help to review, thanks ! The changes include : 1. Add semicolon to the encoding array 2. Add codes to handle those replaced strings, e.g. if the parameter is '0xaabc', make sure '0xa' is not replaced with ? when decoding the parameter value.
        Ivan made changes -
        Field Original Value New Value
        Attachment PLUTO-579.patch [ 12418471 ]
        Hide
        David Jencks added a comment -

        I think there must be a way to only scan through the string once for encoding or decoding. Can we do each operation using only one regular expression?

        Show
        David Jencks added a comment - I think there must be a way to only scan through the string once for encoding or decoding. Can we do each operation using only one regular expression?
        Ivan made changes -
        Attachment PLUTO-579.patch [ 12418471 ]
        Hide
        Ivan added a comment -

        Thanks for the comment, David, I re-created a new patch.

        Show
        Ivan added a comment - Thanks for the comment, David, I re-created a new patch.
        Ivan made changes -
        Attachment PLUTO-579.patch [ 12419531 ]
        Hide
        David Jencks added a comment -

        Can we use a similar regular expression to do the encoding as well as the decoding?

        Show
        David Jencks added a comment - Can we use a similar regular expression to do the encoding as well as the decoding?
        Ivan made changes -
        Attachment PLUTO-579.patch [ 12419531 ]
        Hide
        Ivan added a comment -

        Attach a new patch, it includes:
        a. Use pattern to encode/decode parameter values
        b. Add semicolon to the escape arrays
        c. Add codes to handle 0x0" (which is used to separate multiple parameter values) is contained in the parameter value
        Please help to review the patch, thanks !

        Show
        Ivan added a comment - Attach a new patch, it includes: a. Use pattern to encode/decode parameter values b. Add semicolon to the escape arrays c. Add codes to handle 0x0" (which is used to separate multiple parameter values) is contained in the parameter value Please help to review the patch, thanks !
        Ivan made changes -
        Attachment PLUTO-579.patch [ 12419607 ]
        Hide
        Ivan added a comment -

        Attach a updated patch, including a fix about the dupliate slash while contrcuting the url.
        This may causes some issues while working with Jetty while the compactpath attribute is set with false.

        Show
        Ivan added a comment - Attach a updated patch, including a fix about the dupliate slash while contrcuting the url. This may causes some issues while working with Jetty while the compactpath attribute is set with false.
        Ivan made changes -
        Attachment PLUTO-579-0921.patch [ 12420196 ]
        David Jencks made changes -
        Assignee David Jencks [ djencks ]
        Hide
        David Jencks added a comment -

        I applied this to branches-1.1.x after making it compile with jdk 1.4 settings. (rev 818328). I think it should be applied to trunk to, but I'll ask on the dev list first to make sure its appropriate.

        Show
        David Jencks added a comment - I applied this to branches-1.1.x after making it compile with jdk 1.4 settings. (rev 818328). I think it should be applied to trunk to, but I'll ask on the dev list first to make sure its appropriate.
        David Jencks made changes -
        Fix Version/s 2.0.1 [ 12313984 ]
        Fix Version/s 1.1.8 [ 12313636 ]
        Hide
        Ate Douma added a comment -

        Hi David,

        I looked briefly at both the patch and the current Pluto trunk PortalURLParserImpl.
        While a lot of the code still looks the same, there is one major difference: in trunk parameters are first passed through URLEncoder/URLDecoder before/after the custom encoding/decoding is done like in Pluto 1.1.x.
        As URLEncoder already takes care of ';' characters I think this issue no longer applies to trunk anymore.
        And it seems to me the custom encoding/decoding on trunk could actually now ignore some of the characters currently defined in the ENCODINGS array.
        If using URLEncoder/URLDecoder is as efficient as the new custom encoding/decoding in Pluto 1.1.x branch I don't know.
        So, for performance reasons, it might be interested to review if we should replace the trunk code with this one, but functionally I don't think its needed anymore.

        Show
        Ate Douma added a comment - Hi David, I looked briefly at both the patch and the current Pluto trunk PortalURLParserImpl. While a lot of the code still looks the same, there is one major difference: in trunk parameters are first passed through URLEncoder/URLDecoder before/after the custom encoding/decoding is done like in Pluto 1.1.x. As URLEncoder already takes care of ';' characters I think this issue no longer applies to trunk anymore. And it seems to me the custom encoding/decoding on trunk could actually now ignore some of the characters currently defined in the ENCODINGS array. If using URLEncoder/URLDecoder is as efficient as the new custom encoding/decoding in Pluto 1.1.x branch I don't know. So, for performance reasons, it might be interested to review if we should replace the trunk code with this one, but functionally I don't think its needed anymore.
        Hide
        David Jencks added a comment -

        Hi Ate,

        I now have a lot of questions

        As you note, in trunk both single parameters and multi valued parameters are URLEncoded/Decoded whereas in 1.1.x only single valued paramters were. I think that clearly by the spec (1.0 spec, top of p 32) the multivalued parameters need to be URL encoded/decoded as well in 1.1.x, in other words both before and after the 1.1.x change it's not spec compliant.

        What I don't understand is why encodeCharacters is needed at all, and why we can't always use the URLEncoded/Decoder.

        trunk encodes these characters:

            private static final String[][] ENCODINGS = new String[][] {
             new String[] { "_", "0x1" },
                    new String[] { ".", "0x2" },
                    new String[] { "/", "0x3" },
                    new String[] { "\r", "0x4" },
                    new String[] { "\n", "0x5" },
                    new String[] { "<", "0x6" },
                    new String[] { ">", "0x7" },
                    new String[] { " ", "0x8" },
                    new String[] { "#", "0x9" },
                    new String[] { "?", "0xa" },
                    new String[] { "\\", "0xb" },
                    new String[] { "%", "0xc" },
            };
        

        The URLEncoder docs say everything except these get URL encoded:

        1. The special characters ".", "-", "*", and "_" remain the same.

        "." and "_" are pluto-encoded, "-" and "*" are not.

        pluto encodeCharacters is called:

        after url-encoding multi values. (line 434) This will encode "." and "_" in parameter values. I don't think this is compliant with the spec.

        to encode a windowId (encodeControlParameter, line 395). Is there a regex or similar grammar for window Ids?

        to encode a resourceWindow (toString, line 239). I think this is another windowId.

        to encode the actionWindow (toString, line 248), presumably another windowId.

        to encode resource window cachablilty (toString, line 256). The ResourceURL seems to indicate that the valid cachability values are in a small set of string constants. none of which need encoding.

        to encode resource window resourceId (toString, line 261), presumably another windowId.

        So, depending on what valid windowIds are, the encoding is unnecessary, excessive, or incomplete. I'd like to know what valid windowIds can be.

        Also, there's a problem with strings to be encoded that have the encoding characters in them already, such as "0x1". These need to be re-encoded somehow so decoding produces the original value. I think Ivan's patch does this successfully.

        I'll change the branch impl to use more URLEncoding on multi-values, for the other bits I need more info.

        Show
        David Jencks added a comment - Hi Ate, I now have a lot of questions As you note, in trunk both single parameters and multi valued parameters are URLEncoded/Decoded whereas in 1.1.x only single valued paramters were. I think that clearly by the spec (1.0 spec, top of p 32) the multivalued parameters need to be URL encoded/decoded as well in 1.1.x, in other words both before and after the 1.1.x change it's not spec compliant. What I don't understand is why encodeCharacters is needed at all, and why we can't always use the URLEncoded/Decoder. trunk encodes these characters: private static final String [][] ENCODINGS = new String [][] { new String [] { "_" , "0x1" }, new String [] { "." , "0x2" }, new String [] { "/" , "0x3" }, new String [] { "\r" , "0x4" }, new String [] { "\n" , "0x5" }, new String [] { "<" , "0x6" }, new String [] { ">" , "0x7" }, new String [] { " " , "0x8" }, new String [] { "#" , "0x9" }, new String [] { "?" , "0xa" }, new String [] { "\\" , "0xb" }, new String [] { "%" , "0xc" }, }; The URLEncoder docs say everything except these get URL encoded: The special characters ".", "-", "*", and "_" remain the same. "." and "_" are pluto-encoded, "-" and "*" are not. pluto encodeCharacters is called: after url-encoding multi values. (line 434) This will encode "." and "_" in parameter values. I don't think this is compliant with the spec. to encode a windowId (encodeControlParameter, line 395). Is there a regex or similar grammar for window Ids? to encode a resourceWindow (toString, line 239). I think this is another windowId. to encode the actionWindow (toString, line 248), presumably another windowId. to encode resource window cachablilty (toString, line 256). The ResourceURL seems to indicate that the valid cachability values are in a small set of string constants. none of which need encoding. to encode resource window resourceId (toString, line 261), presumably another windowId. So, depending on what valid windowIds are, the encoding is unnecessary, excessive, or incomplete. I'd like to know what valid windowIds can be. Also, there's a problem with strings to be encoded that have the encoding characters in them already, such as "0x1". These need to be re-encoded somehow so decoding produces the original value. I think Ivan's patch does this successfully. I'll change the branch impl to use more URLEncoding on multi-values, for the other bits I need more info.
        Hide
        Ate Douma added a comment -

        Hi David,

        Sorry for not replying any time sooner on your questions.
        I agree definitely the current 1.1.x and possibly also the trunk parameter encoding might not be fully compliant yet and in some areas seems to be unnecessary and excessive indeed.
        But I still haven't had time to look deeper into this, nor will I be able to do so this week.
        I never really worked on the Pluto 1.1.x code base and for 2.0 only migrated and fixed (the Pluto Driver) code where minimally needed.
        If you already have progressed further since your last comment, please let me know: I'd like to further discuss and help out to get this straightened out.
        I'll try to chime back in on this issue next week if possible. If you want, ping me in private to remind me

        Show
        Ate Douma added a comment - Hi David, Sorry for not replying any time sooner on your questions. I agree definitely the current 1.1.x and possibly also the trunk parameter encoding might not be fully compliant yet and in some areas seems to be unnecessary and excessive indeed. But I still haven't had time to look deeper into this, nor will I be able to do so this week. I never really worked on the Pluto 1.1.x code base and for 2.0 only migrated and fixed (the Pluto Driver ) code where minimally needed. If you already have progressed further since your last comment, please let me know: I'd like to further discuss and help out to get this straightened out. I'll try to chime back in on this issue next week if possible. If you want, ping me in private to remind me
        Hide
        Ate Douma added a comment -

        Bumping release target from 2.0.1 to 2.0.2

        Show
        Ate Douma added a comment - Bumping release target from 2.0.1 to 2.0.2
        Ate Douma made changes -
        Fix Version/s 2.0.2 [ 12314872 ]
        Fix Version/s 2.0.1 [ 12313984 ]
        Hide
        Ate Douma added a comment -

        Without further input and anticipating the 2.0.2 release shortly, I'm bumping this issue again, to 2.0.3 for now

        Show
        Ate Douma added a comment - Without further input and anticipating the 2.0.2 release shortly, I'm bumping this issue again, to 2.0.3 for now
        Ate Douma made changes -
        Fix Version/s 2.0.3 [ 12315094 ]
        Fix Version/s 2.1.0 [ 12315040 ]
        Fix Version/s 2.0.2 [ 12314872 ]
        Mark Thomas made changes -
        Workflow jira [ 12474780 ] Default workflow, editable Closed status [ 12565130 ]
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12565130 ] jira [ 12585811 ]
        Hide
        Ate Douma added a comment -

        bumping it further to 2.0.4 as still waiting on further input

        Show
        Ate Douma added a comment - bumping it further to 2.0.4 as still waiting on further input
        Ate Douma made changes -
        Fix Version/s 2.0.4 [ 12317962 ]
        Fix Version/s 2.0.3 [ 12315094 ]

          People

          • Assignee:
            David Jencks
            Reporter:
            Ivan
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development