Deep links regular expressions collision [184072811]

Fixed

Bug

androidx-bug-bounty

Status Update

No update yet.

Description

[Deleted User]

created issue #1

Mar 31, 2021 12:57PM

Artifact used: androidx.navigation:navigation-fragment-ktx:2.3.4
Version used: 2.3.4
Theme used: N/A
Devices/Android versions reproduced on: Any

Problem

We have the following navigation graph:

<?xml version="1.0" encoding="utf-8"?>
<navigation xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto"
    xmlns:tools="http://schemas.android.com/tools"
    android:id="@+id/profile_graph"
    app:startDestination="@id/profileFragment">

    <fragment
        android:id="@+id/profileFragment"
        android:name="com.myapp.profile.ProfileFragment"
        tools:layout="@layout/fragment_profile">

        <argument
            android:name="userId"
            app:argType="string" />

        <action
            android:id="@+id/toProfileDetails"
            app:destination="@id/profileDetailsFragment" />

        <deepLink
            android:id="@+id/deepLinkProfile"
            app:uri="myapp://profiles/{userId}/" />

    </fragment>

    <fragment
        android:id="@+id/profileDetailsFragment"
        android:name="com.myapp.profile.ProfileDetailsFragment"
        tools:layout="@layout/fragment_profile_details">

        <argument
            android:name="userId"
            app:argType="string" />

        <deepLink
            android:id="@+id/deepLinkProfileDetails"
            app:uri="myapp://profiles/{userId}/details/" />

    </fragment>
</navigation>

Here are declared two deep links patterns:

myapp://profiles/{userId}/
myapp://profiles/{userId}/details/

But the second deep link will never be handled because these deep links are "equal" for navigation when it tries to find the best matching deep link. In this case, NavGraph will always use first matching NavDestination (profileFragment).

For example link myapp://profiles/u2/details/ will be handled as myapp://profils/{userId}/ where userId = u2/details.

There is a workaround - remove ending slash in second pattern to make patterns tails not matching:

myapp://profiles/{userId}/
myapp://profiles/{userId}/details

It works but it is not obvious.

Purposed solution

Make arguments to not include / character.

In resulting regexp: ^\Qmyapp://profiles/\E(.+?)\Q/\E($|(\?(.)*))
Replace: (.+?)
With: ([^/]+?)

If this solution is right, I can create pull request on GitHub.

Comments

jb...@google.com <jb...@google.com> #2Apr 2, 2021 02:33PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

[Deleted User] <[Deleted User]> #3Apr 3, 2021 10:23AM

I've created a pull request on GitHub. Can you check it?

I not sure about names picked for test functions.

[Deleted User] <[Deleted User]> #4Apr 13, 2021 09:57AM

When will you have time to review this pull request? This change is important for our project.

ap...@google.com <ap...@google.com> #5Apr 15, 2021 06:58PM

Project: platform/frameworks/support
Branch: androidx-main

commit 4eea9cb436ba82a6f61ff97a69bbe7c393df0132
Author: Osip Fatkullin <osip.fatkullin@gmail.com>
Date: Thu Apr 15 09:34:16 2021

[GH] [Navigation] Make deep link path arguments to not include slash

## Proposed Changes

Slash is used for URI path splitting. Arguments should not include more than one path section.
I purpose to replace `(.+?)` with `([^/]+?)` in resulting regexp.

## Testing

Added corresponing tests to `NavDestinationAndroidTest`

Test: ./gradlew test connectedCheck

## Issues Fixed

Fixes: [

b/184072811

](

https://issuetracker.google.com/issues/184072811)

This is an imported pull request from

https://github.com/androidx/androidx/pull/153.

Resolves #153
Github-Pr-Head-Sha: f16861bc1f6b0bbba03e752cc378aa795a8522e7
GitOrigin-RevId: 5da0cf583393bce4f68ab11305f1bab3d89f0480
Change-Id: I9f147ead04b611febd2a3214cf8736b5a9afcc65

M navigation/navigation-common/src/androidTest/java/androidx/navigation/NavDestinationAndroidTest.kt
M navigation/navigation-common/src/main/java/androidx/navigation/NavDeepLink.kt

https://android-review.googlesource.com/1676867

il...@google.com <il...@google.com> May 4, 2021 08:05PM

Marked as fixed, reassigned to jb...@google.com.