Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ios): proper handling of allowNavigation with multiple wildcard #5096

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

macdja38
Copy link
Contributor

@macdja38 macdja38 commented Oct 4, 2021

At the moment allowed hosts with multiple wildcards are processed in-correctly
For example
Pattern: *.*.example.com
Host: test1.test2.example.com

Iteration 1 wildcard.offset is 0
Pattern becomes: *.example.com
Host becomes: test2.example.com

Iteration 2 wildcard.offset is 1
Pattern becomes: *.com
Host becomes: test2.com

The correct outcomes would be
Pattern: example.com
Host: example.com

Reversing the direction of iteration fixes this, as removing the item at index 1 first is safe and doesn't effect the next step of removing the item at index 0.

At the moment
Pattern: `*.*.example.com`
Host: `test1.test2.example.com`

Iteration 1 wildcard.offset is 0
Pattern becomes: `*.example.com`
Host becomes: `test2.example.com`

Iteration 2 wildcard.offset is 1
Pattern becomes: `*.com`
Host becomes:  `test2.com`

The correct outcomes would be
Pattern: `example.com`
Host: `example.com`
@macdja38
Copy link
Contributor Author

macdja38 commented Oct 4, 2021

This is actually the old behaviour, the following code was removed ~10 months ago.

let wildcards = pattern.enumerated().filter { $0.element == "*" }
         for wildcard in wildcards.reversed() {
             host.remove(at: wildcard.offset)
             pattern.remove(at: wildcard.offset)
         }

         return host == pattern

Should I add a comment to ensure it's not removed again?

@macdja38
Copy link
Contributor Author

macdja38 commented Oct 4, 2021

Added a test instead of a comment.

@StareInTheAir
Copy link

I found this exact bug on Friday and wanted to create a PR just now, with the almost exact same fix. You beat me to it :)

@jcesarmobile jcesarmobile reopened this Oct 6, 2021
@jcesarmobile
Copy link
Member

We have a setting that doesn't allow us to merge if the branch is not in sync with main branch, I usually see a button to update the branch, but not in your PR, did you check the checkbox to allow changes from maintainers when you created the PR?

Anyway, you probably see the button to update the branch, can you click it?

@jcesarmobile jcesarmobile changed the title Fix multiple wildcard processing fix(ios): proper handling of allowNavigation with multiple wildcard Oct 6, 2021
@macdja38
Copy link
Contributor Author

We have a setting that doesn't allow us to merge if the branch is not in sync with main branch, I usually see a button to update the branch, but not in your PR, did you check the checkbox to allow changes from maintainers when you created the PR?

Anyway, you probably see the button to update the branch, can you click it?

Clicked!

@jcesarmobile jcesarmobile merged commit cda17a6 into ionic-team:main Oct 11, 2021
@jcesarmobile
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants