publicsuffix / list

The Public Suffix List
https://publicsuffix.org/
Mozilla Public License 2.0
1.93k stars 1.18k forks source link

Fix test case for exception rule #1998

Closed simon-friedberger closed 1 month ago

simon-friedberger commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/rockdaboot/libpsl/issues/48#issuecomment-184582216 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 commented 1 month ago

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 https://github.com/publicsuffix/list/issues/1989#issuecomment-2168290448

simon-friedberger commented 1 month ago

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

ko-zu commented 1 month ago

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 commented 1 month ago

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