Magic project URL can crash app

[seadowg]

Software and hardware versions

Collect v2021.2 and later

Problem description

Certain magic URLs can crash the app when "Add" is pressed (or presumably if they are imported from QR code).

Steps to reproduce the problem

1. Go to add a new project
2. Tap "Manually enter project details"
3. Enter https:///. as the URL
4. Tap "Add"

The app crashes.

Expected behavior

It shouldn't be possible for any URL to crash the app

Other information

• This happens because https:///. beats our URL validation (it's a http(s) protocol followed by at least one dot) but the host will be blank when parsed as a URL (due to the third /). The host ("") will then be used as the project name and that will be used to create the icon which causes the crash.

• Crashlytics report is here

• Stacktrace of crash:

  Fatal Exception: java.util.NoSuchElementException: Char sequence is empty.
         at kotlin.text.StringsKt___StringsKt.first(StringsKt___StringsKt.java:71)
         at kotlin.text.StringsKt.first(StringsKt.java)
         at org.odk.collect.android.projects.ProjectDetailsCreator.createProjectFromDetails(ProjectDetailsCreator.java:24)
         at org.odk.collect.android.configure.SettingsImporter.importProjectDetails(SettingsImporter.java:131)
         at org.odk.collect.android.configure.SettingsImporter.fromJSON(SettingsImporter.java:60)
         at org.odk.collect.android.projects.ProjectCreator.createNewProject(ProjectCreator.java:16)
         at org.odk.collect.android.projects.ManualProjectCreatorDialog.createProject(ManualProjectCreatorDialog.java:210)
         at org.odk.collect.android.projects.ManualProjectCreatorDialog.handleAddingNewProject(ManualProjectCreatorDialog.java:179)
         at org.odk.collect.android.projects.ManualProjectCreatorDialog.onViewCreated$lambda-6(ManualProjectCreatorDialog.java:135)
         at org.odk.collect.android.projects.ManualProjectCreatorDialog.$r8$lambda$7ra-bH8R3mE4z2po42zZKmy8brA(ManualProjectCreatorDialog.java)
         at org.odk.collect.android.projects.ManualProjectCreatorDialog$$InternalSyntheticLambda$0$defaa1d6c35fd3b8172bb396b0a10a1633d81b1bd70a0bb99b3f3fdd9059e099$2.onClick(ManualProjectCreatorDialog.java:2)
         at android.view.View.performClick(View.java:7870)
         at android.widget.TextView.performClick(TextView.java:14970)
         at com.google.android.material.button.MaterialButton.performClick(MaterialButton.java:1119)
         at org.odk.collect.android.views.multiclicksafe.MultiClickSafeButton.performClick(MultiClickSafeButton.java:16)
         at android.view.View.performClickInternal(View.java:7839)
         at android.view.View.access$3600(View.java:886)
         at android.view.View$PerformClick.run(View.java:29363)
         at android.os.Handler.handleCallback(Handler.java:883)
         at android.os.Handler.dispatchMessage(Handler.java:100)
         at android.os.Looper.loop(Looper.java:237)
         at android.app.ActivityThread.main(ActivityThread.java:7860)
         at java.lang.reflect.Method.invoke(Method.java)
         at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
         at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1075)
  

[seadowg]

I don't think this is high priority, but I think we should get into a patch for v2021.3 in case someone has some weird magic URL (with the same "host is technically blank" problem) we haven't though of that's causing this crash. It's not happening often at all and existed in v2021.2 as far as I can see, but it's nasty to have a possible crash that results from a typo during a critical setup flow.

There are a few options for fixing this, but I'm in favour of making our name, icon generation more robust instead of adding more validation. I'm already a little worried that our validation is too strict - if a device is using a network with DNS setup to resolve something like http://odk-central then we'd currently not allow that.

Thoughts @grzesiek2010 @yanokwa?

[yanokwa]

And here I thought the validation was not strict enough. I'm open to making the regex http://[.], but what are you proposing when you say you want to make the name and icon generation more robust?

[seadowg]

And here I thought the validation was not strict enough

I guess with cases like this I'm always paranoid about blocking something genuine. Does my "network with weird DNS" scenario not worry you? Or you, just thinking that stricter validation is probably more useful for most users people (a valid argument)? My preference would be to just parse it as a URL in Java and validate the protocol (as we only support http(s) traffic). Ultimately that's all we need to make sure the string is going to work throughout our http stack.

In the long term, we've talked about validating the URL can connect to an Open Rosa server on this screen which I think is better than trying to enforce a structure.

but what are you proposing when you say you want to make the name and icon generation more robust?

I guess whatever way we go, I think we should add tests to ProjectDetailsCreator to cover this magic string and fix the explosion. That code shouldn't explode in any situation. If we went with my preference of just doing a URL parse (or just didn't change the validation) we'd end up having to fix this there as the magic string would still be a valid URL.

[grzesiek2010]

I think we should add tests to ProjectDetailsCreator to cover this magic string and fix the explosion

I agree. First we should add more tests ad then we can use one of the options: URLUtil.isValidUrl or Patterns.WEB_URL or both (whatever satisfies our tests).