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 test case for exception rule #1998

Closed

Conversation

simon-friedberger
Copy link
Contributor

As pointed out in #1989 there seems to be a bug in a test case where something that has an explicit exception is returned as a public suffix. This fixes that test case.

@simon-friedberger
Copy link
Contributor Author

At least in Firefox it behaves as I would expect:

Services.eTLD.getPublicSuffixFromHost("city.kobe.jp")
"kobe.jp"
Services.eTLD.getPublicSuffixFromHost("www.city.kobe.jp")
"kobe.jp" 

if anybody knows how to easily test in other browsers please let me know!

@simon-friedberger
Copy link
Contributor Author

Note, this code expects the public suffix:

function checkPublicSuffix(host, expectedSuffix)
{
  var actualSuffix = null;
  try {
    actualSuffix = etld.getBaseDomainFromHost(host);
  } catch (e if e.result == Cr.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS ||
                e.result == Cr.NS_ERROR_ILLEGAL_VALUE) {
  }
  // The EffectiveTLDService always gives back punycoded labels.
  // The test suite wants to get back what it put in.
  if (actualSuffix !== null && expectedSuffix !== null &&
      /(^|\.)xn--/.test(actualSuffix) && !/(^|\.)xn--/.test(expectedSuffix)) {
    actualSuffix = idna.convertACEtoUTF8(actualSuffix);
  }
  do_check_eq(actualSuffix, expectedSuffix);
}

while the discussion here rockdaboot/libpsl#48 (comment) indicates that this function may in the past have returned the shortest registrable part. So maybe it was improperly converted at some point?

@ko-zu
Copy link
Contributor

ko-zu commented Jun 14, 2024

I think the existing test case is correct and should not be altered. The algorithm described in the wiki should be corrected to match the test case. As I commented in
#1989 (comment)

@simon-friedberger
Copy link
Contributor Author

Could you elaborate a bit? I don't understand how something that has an explicit exception rule could possibly be an expectedSuffix.

@ko-zu
Copy link
Contributor

ko-zu commented Jun 14, 2024

checkPublicSuffix('city.kobe.jp', 'city.kobe.jp');
checkPublicSuffix('www.city.kobe.jp', 'city.kobe.jp');

Services.eTLD.getPublicSuffixFromHost("city.kobe.jp")
"kobe.jp"
Services.eTLD.getPublicSuffixFromHost("www.city.kobe.jp")
"kobe.jp" 

if anybody knows how to easily test in other browsers please let me know!

!city.kobe.jp declares that it is not a public suffix. The longest public suffix here is kobe.jp implied by *.kobe.jp. Firefox follows the linter's rule and the test case, not the wiki.
Found examples from MDN mirror, https://udn.realityripple.com/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIEffectiveTLDService#getPublicSuffix()

the checkPublicSuffix() example indicates that the longest public suffix +1 label on top is city.kobe.jp, and kobe.jp is the longest public suffix.

@simon-friedberger
Copy link
Contributor Author

Oh, the naming here is just confusing expectedSuffix and actualSuffix are not public suffixes but actually eTLD+1. Thanks for pointing this out!

@weppos weppos changed the title Fix test case for exception rule. Fix test case for exception rule Jun 15, 2024
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.

2 participants