navigateTo + ViewChangeEvent.parameterList may double URL decode

[DALDEI]

If I use parameters that require URL encoding in Navigator.navigateToView(), a subsequecent ViewChangeEvent.parameterList may double -URL decode depending on if this is a direct in-app navigation or if it is the result of browser-side request (refresh, bookmark, manual enter).

Example:

navigateToView( DocstoreTreeView::class.java , item.url )

where item.url is a string in URL format such as https://somewhere/over/the/rainbow

the corresponding enter events parameterList may be either

[ "0" -> "https://somewhere/over/the/rainbow"]
Or
[ "0" -> "https" , "1" -> "somewhere/over/the/rainbow" ]

I tracked this down to largely correct (*) code that encodes on navigate and decodes on enter,
the cause seems to be Vaadin code which in the direct case supplies the ENCODED string "parameters" but in the refresh/manual case supplies the DECODED string. Which then vok decodes again.

In many cases this doesn't matter, in the above case there is 1 bug from the extra decode and 1 bug from vok implementation

1. the double-decode causes the "/" to be interpreted as a seperator so you get a split string
2. intentional code in parameterList removes the empty argument caused by ("//")
3. an encoded % escape would be interpreted differently

#1 might be able to work around if #2 were not an issue. (#2 cannot determine between "/" or "//" being stripped)

Suggest that at minimal the code in parameterList be changed to NOT remove empty parameters, this would allow a workaround of the vaadin issue (not able to determine core cause yet)

[DALDEI]

Forgot to mention:
Workaround: use parameters instead of parameterList in calling code, still suffers from inconsistent 0 or 1 decoding but not from double decoding.

[mvysny]

Hi, thanks for letting me know! I have created a test case for this bug, but I'm unable to reproduce it in a mocked environment. Can you look at this test please and maybe try to make it fail? https://github.com/mvysny/karibu-dsl/blob/master/karibu-dsl-v8/src/test/kotlin/com/github/vok/karibudsl/NavigatorTest.kt

Yet from what you say, it seems that Vaadin is doing something wrong and it is only shown when the request goes through an actual browser...

[DALDEI]

Yes I will try your test but in my cases it only breaks when using a bookmarked or manual history or a hard refresh where the URL comes through some other path (maybe even javascript).

[DALDEI]

I tried the following as a top level view.
Enter a URL or string containing '/' into the text field and hit PUSH

Result: (given "http://foo/bar" )

   rawParams: http%3A%2F%2Ffoo%2Fbar
      params: {0=http://foo/bar}

How go to browser bar and hit REFRESH

Results: (wrong/inconsistant)
rawParams: http://foo.bar/spam
params: {0=http:, 1=foo.bar, 2=spam

For fun keep pressing PUSH ... this simulates doing the URL Encode prior to passing to navigateToView
the URL gets iteratively encoded (expected but useless).

package com.github.vok.karibudsl

import com.vaadin.navigator.Navigator
import com.vaadin.navigator.View
import com.vaadin.navigator.ViewChangeListener
import com.vaadin.navigator.ViewDisplay
import com.vaadin.ui.*
import jdk.management.resource.NotifyingMeter

@AutoView
class SimpleView : View, VerticalLayout() {
    val params: MutableMap<Int, String> = mutableMapOf()
    var rawParameters = ""
    lateinit var txt : TextField
    lateinit var lab : Label


    init {
      lab = label  {

      }

        txt = textField("Put SINGLE URL Here as a parameter") {
          value = rawParameters
        }
        button("Push") {
          addClickListener {
            navigateToView(SimpleView::class.java, txt.value as String )
          }
        }

    }

  override fun enter(event: ViewChangeListener.ViewChangeEvent) {
    rawParameters = event.parameters
    params.putAll(event.parameterList)
    txt.value = rawParameters
    lab.html( """<pre>
          rawParams: ${rawParameters}
          params: ${params}
          </pre>""")
  }

}

[mvysny]

You're right, the outcome looks pretty random to me. I tried to remove the VoK-related code, to see how pure Vaadin would behave. The behavior is even more random. I have the following app:

@PushStateNavigation
class MyUI : UI() {
    @Override
    override fun init(vaadinRequest: VaadinRequest?) {
        navigator = Navigator(this, this)
        navigator.addProvider(autoViewProvider)
    }
}

@AutoView("my")
class MyView : View, VerticalLayout() {
    override fun enter(event: ViewChangeListener.ViewChangeEvent) {
        label("The parameter was ${event.parameters}")
        val paramtf = textField("Enter new parameter")
        button("Go!") {
            onLeftClick { UI.getCurrent().navigator.navigateTo("my/${paramtf.value}") }
        }
    }
}

• Scenario 1: type in "foo". After you press the Go button, "foo" is properly displayed. After you reload the browser using F5, "foo" is properly displayed. This one works.
• Scenario 2: type in "foo/bar". After you press the Go button, "foo/bar" is properly displayed. After you reload the browser using F5, "foo/bar" is properly displayed. This one also works.
• Scenario 3: type in "foo//bar". After you press the Go button, "foo//bar" is properly displayed. After you reload the browser using F5, the page breaks completely, saying that the widgetset can not be loaded! It looks as if absolutely no escaping is attempted by Vaadin itself.
• Scenario 4: type in "http:/bar". After you press the Go button, "http:foo/bar" is properly displayed. After you reload the browser using F5, "http:/bar" is properly displayed. This seems to be working.
• Scenario 5: type in "http%3A%2F". After you press the Go button, "http%3A%2F" is properly displayed. After you reload the browser using F5, Tomcat fails and shows error 400 Invalid URI: noSlash: " The server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing)."
• Scenario 6: type in "http%3A". After you press the Go button, "http%3A" is properly displayed. After you reload the browser using F5, the "http:" is displayed. In this case it seems that Vaadin is not doing escaping in Navigator.navigate yet it's unescaping when navigated to that view.

I am somehow at loss as to what to make of the above. It seems that double slash kills Vaadin reliably, yet %2F in path killing Tomcat is something new. Yet it seems that there is a bug in Vaadin in sense that it's not doing escaping in navigator.navigate yet it's unescaping the parameter when the browser is refreshed via F5. Looks like a bug in Vaadin to me, but maybe it's intentional so that navigate() wouldn't encode slashes...

[mvysny]

Also it seems it's impossible to pass double-slash via a parameter: if you pass it unescaped, Vaadin widgetset will fail to be loaded. If you pass it escaped, Tomcat will complain with error 400 ;) Pick your poison... I think the path-parameters were meant for simple string passing only - if you need to pass anything more complex, you need to use query parameters. Now the question is how to make Vaadin Navigator to pass in query parameters...

[mvysny]

Circling back to the original bug: you're right. It seems that Vaadin does not do escaping at all on navigate(); in-app navigation would then not perform any unescaping yet F5 would perform unescaping. This is strange duality that's reproduced by Scenario 6 and I think it deserves a bug report for Vaadin :) It would be great to provide a simple Java-only Vaadin 8 app which simulates this bug.

[mvysny]

The workaround seems to be to remove the @PushStateNavigation - this seems to cause the escaping issue to be fixed.

[mvysny]

Filed a bug: vaadin/framework#11057

[mvysny]

Also related: vaadin/framework#11060

[DALDEI]

Interesting wrt @PushStateNavigation --- nievely guessing that is due to the URL being (or not) processed by Tomcat (or jetty in my case) vs being 'data' in the push stream and processed by vaadin (or not).
Changing to no-push would imply more consistently correct behaviour as tomcat and most web servers have addressed this issue really really early on. URL Escaping is a doozy -- its barely deterministic, in fact in this case its not defined as the path component is strictly a scheme and implementation issue -- but -- the http: scheme, even on non-standard OS's tends to be implemented consistently. In a query parameter maybe slightly more defined.
FYI: I solved this by borrowing some code from apache commons, theres a function that does a 'maybeDecodePath' (names vary by version). It peeks into the string looking for + or %, and only if it finds them does a URL decode. same on encode. This solves a good portion of the problem set., if you stay away from % in non-encoded paths :)

[DALDEI]

Maybe this can be moved to a "Feature Request" by adding support for typed parameters.
Most of my code has ended up using the same boilerplate, rougthly

class myView() : View  {
...
  fun init( resourceId: String , [ sometimes more args like pageNum: ]){
     val object = loadMyObject(resourceId).atPage(number) ..    
     . . create form with supplied value
  }
  override fun enter( ..) ... {  
      ... Extract and decode parameters using some reliably URL safe scheme
     init( parsed arguments )
  }
  companion object {
      fun navigateTo( resourceId: String , someOtherParam: Type) {
                navigateTo<MyView>(  encodeMyStufToSafeURLString( resourceId, someOtherParam) )
     }
}

As I enhance the above to handle cases like a form refresh ( requires saving previous args), general purpose 'goToEditorForObject()" more becomes boilerplate and eventually pushed downto a common view subtype.

Seems like a better aproach is deeper integration into the navigator that supports 'safe typed values' , i.e. a navigation API like

inline fun <reified T:Any,reified V:View> navigateTo( arg: T ) {
navigateTo(V::class, serializeObject(arg) )
}
...

interface FancyView : View {

   override fun enter( args .. ) {
       fancyEnter(  decodeObject( args )  )
  }
  fun <T> fancyEnter( arg: T ) { 

  }
}

< hand waving -- details are tricky >

• reified generics for clases :( -- how to define the various functions just right
• serialization vs memoization vs value vs reference --> not all argument types have a reasonable serialization directly, nor a reasonable 'handle' -- so may require memoization