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

Add support for wildcard and exception #29

Merged

Conversation

Suven-p
Copy link
Contributor

@Suven-p Suven-p commented Apr 12, 2023

Fix #22
Fix appwrite/appwrite#5317

Appwrite relies on public suffix list to parse domain names. According to the format specification:

  1. The wildcard character * matches any valid sequence of characters in a hostname part.
  2. An exclamation mark (!) at the start of a rule marks an exception to a previous wildcard rule. An exception rule takes priority over any other matching rule.

Current behaviour:
The current implementation does not handle both cases. It handles suffixes without * and ! which covers all common use cases but excludes some country tld's such as *.np.

Expected behaviour:

  • With rule: *.np suffix for example.com.np is com.np
  • With rules: *.kawasaki.jp and !city.kawasaki.jp suffix for app.example.kawasaki.jp is example.kawasaki.jp and suffix for app.city.kawasaki.jp is kawasaki.jp

@Suven-p
Copy link
Contributor Author

Suven-p commented Apr 12, 2023

The tests for OpenSRS fails but I haven't modified anything related to OpenSRS.

@Suven-p
Copy link
Contributor Author

Suven-p commented Apr 13, 2023

@TorstenDittmann Could you please review this?

@Suven-p
Copy link
Contributor Author

Suven-p commented Apr 19, 2023

@wess Could you review this 😅? I can't currently use my domain since it's not supported appwrite/appwrite#5317

@wess
Copy link
Contributor

wess commented Apr 19, 2023

The tests for OpenSRS fails but I haven't modified anything related to OpenSRS.

Did you pull in the latest master? There was an issue with that test but it should be addressed.

@wess
Copy link
Contributor

wess commented Apr 19, 2023

Could you please add a test (or 2) for testing the rules?

@wess wess self-assigned this Apr 19, 2023
@Suven-p Suven-p force-pushed the fix-22-support-wildcard-and-exceptions branch from bfe3236 to 741bd5e Compare April 19, 2023 14:37
@Suven-p
Copy link
Contributor Author

Suven-p commented Apr 19, 2023

Did you pull in the latest master? There was an issue with that test but it should be addressed.

I think so. The hash of third last commit from my branch matches with master.

@wess
Copy link
Contributor

wess commented Apr 19, 2023

Did you pull in the latest master? There was an issue with that test but it should be addressed.

I think so. The hash of third last commit from my branch matches with master.

Thank you for checking, if you could add those tests then i can also run the tests to see if there is any branch drift. Thanks again for the contribution, this is a killer feature.

@Suven-p
Copy link
Contributor Author

Suven-p commented Apr 19, 2023

The last commit adds the tests.

@Suven-p
Copy link
Contributor Author

Suven-p commented Apr 27, 2023

@wess Any updates?

@stnguyen90 stnguyen90 requested review from christyjacob4 and wess and removed request for christyjacob4 May 4, 2023 16:25
@stnguyen90
Copy link

@wess, should the travis build pass?

Copy link
Contributor

@wess wess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and no issues on my end. @TorstenDittmann should this type of update be required to pass Travis?

Copy link
Contributor

@wess wess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and no issues on my end. @TorstenDittmann should this type of update be required to pass Travis?

Copy link

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be able to add some additional test cases?

  • *.nom.br
  • *.kawasaki.jp
  • !city.kawasaki.jp
  • *.dev.adobeaemcloud.com
  • adobeaemcloud.net

@stnguyen90 stnguyen90 self-requested a review July 19, 2023 16:28
Copy link

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey 👋 awesome work on your PR! We've approved your work and it'll be merged soon!

@eldadfux eldadfux merged commit b9b573f into utopia-php:master Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: Country tld for .np is not supported 🐛 Bug Report: getSuffix() empty for some domains
5 participants