security(config): refuse any '@' in a published URL, not just userinfo

Changelog.txt

@@ -77,6 +77,20 @@ Newest first. Dates are ISO 8601 (YYYY-MM-DD).
     entry (the official schema pins only the first two and allows extras); the
     context is pinned in the fail-closed allowlist and covered by the drift
     watchdog (#239).
+  - security(config): close the bypass in the userinfo guard below: checking
+    `urlsplit(...).netloc` only catches a credential whose `@` lands inside the
+    authority, and a password holding a `/`, `?` or `#` pushes it out. Both
+    layers accepted `https://user:12345/x@host/` — which `did_web_from_url`
+    then turned into the issuer id `did:web:user%3A12345:x%40host`, carried by
+    every signed credential — and `https://user:secret/x@host/`, which surfaced
+    the password through urllib's own `Port could not be cast to integer value
+    as 'secret'` (a message the redaction added below could not reach, since
+    urllib raises it). Both layers now refuse an `@` or `%40` *anywhere* in a
+    URL-shaped value, and `did_web_from_url` wraps the port parse and quotes
+    nothing at all. Values with no authority — `[issuer] email`, local paths —
+    still accept an `@`. BREAKING (narrow): a `publish_url` whose path contains
+    an `@`, e.g. `https://host/@name/`, is now refused; once parsed it is
+    indistinguishable from one hiding a credential (#255).
   - security(config): reject a `[issuer]` / `[badge_*]` URL that embeds
     userinfo (`https://user:password@host/`) when the config is loaded. Those
     URLs are public identifiers: `publish_url` is printed by every publish

openbadgeslib/config.ini.example

@@ -30,7 +30,10 @@ mail_from = no-reply@issuer.badge
 ; Every URL here and in the badge sections is public: it is printed by the
 ; tools, published in the badge metadata and embedded in signed credentials.
 ; A URL that embeds credentials (https://user:password@host/) is rejected when
-; the config is loaded, so the password cannot leak into any of them.
+; the config is loaded, so the password cannot leak into any of them. The rule
+; is blunt on purpose: an '@' (or %40) ANYWHERE in one of those URLs is
+; refused, because a password holding a '/' hides the '@' from the parser.
+; Values that are not URLs (the e-mail below, local paths) are unaffected.
 [issuer]
 name           = OpenBadge issuer
 url            = https://www.issuer.badge

openbadgeslib/confparser.py

@@ -290,7 +290,7 @@ def badge_section_config(conf: ConfigParser,
 
 
 def reject_url_userinfo(parser: ConfigParser, config_file: str) -> None:
-    """Reject any [issuer]/[badge_*] value that is a URL carrying userinfo.
+    """Reject any [issuer]/[badge_*] value that is a URL containing an ``@``.
 
     Those sections hold the identifiers the tools publish: ``publish_url`` is
     printed by every publish command, joined into the ids written to
@@ -303,6 +303,18 @@ def reject_url_userinfo(parser: ConfigParser, config_file: str) -> None:
     and every consumer (OB1/OB2/OB3, DID derivation, status lists) inherits the
     guarantee instead of re-checking it.
 
+    A published identifier has no legitimate use for an ``@``, so the whole URL
+    is searched rather than the userinfo component alone: ``urlsplit`` only
+    reports userinfo when the ``@`` sits inside the authority, and a password
+    holding a ``/``, ``?`` or ``#`` pushes it into the path, where the parser
+    stops seeing it as a credential but every downstream consumer still
+    publishes it verbatim. ``%40`` is refused for the same reason. The cost is
+    that an innocent ``https://host/@name/`` is refused too — deliberate: after
+    parsing, the two are the same shape.
+
+    A value with no authority is left alone: an ``[issuer] email`` or a local
+    path may legitimately hold an ``@`` and is not a dereferenceable URL.
+
     The message names the offending section and key but never echoes the value:
     printing it would leak the very password the check exists to protect.
     """
@@ -312,21 +324,22 @@ def reject_url_userinfo(parser: ConfigParser, config_file: str) -> None:
                 and not section.startswith(_PUBLIC_URL_SECTION_PREFIX)):
             continue
         for key, value in parser.items(section):
+            if '@' not in value and '%40' not in value.lower():
+                continue
             try:
-                authority = urlsplit(value).netloc
+                has_authority = bool(urlsplit(value).netloc)
             except ValueError:
-                # Too malformed for urlsplit (an unbalanced IPv6 bracket).
-                # Read the authority textually rather than skip the value:
-                # it is still written out verbatim downstream.
-                authority = value.partition('//')[2].split('/', 1)[0]
-            if '@' in authority:
+                # Too malformed for urlsplit (an unbalanced IPv6 bracket) to
+                # report an authority. A '//' still makes it a URL, and it is
+                # written out verbatim downstream, so judge it as one.
+                has_authority = '//' in value
+            if has_authority:
                 raise ConfigError(
-                    "Configuration file %s: [%s] %s must not carry userinfo "
-                    "(user:password@host) in its URL. That URL is printed by "
-                    "the tools, published in badge metadata and embedded in "
-                    "signed credentials, so the credential would leak; drop "
-                    "the userinfo and protect the host another way."
-                    % (config_file, section, key))
+                    "Configuration file %s: [%s] %s must not contain '@' in "
+                    "its URL. That URL is printed by the tools, published in "
+                    "badge metadata and embedded in signed credentials, so a "
+                    "'user:password@' there would leak; a published identifier "
+                    "has no legitimate use for an '@'." % (config_file, section, key))
 
 
 class ConfParser():

openbadgeslib/ob3/did.py

@@ -280,29 +280,52 @@ def did_web_from_url(url: str) -> str:
     Exact inverse of the resolution above: the host keeps any port
     percent-encoded, path segments join with ':', and a bare host resolves
     at ``/.well-known/did.json`` while a path resolves at ``<path>/did.json``.
-    Raises ValueError for a non-HTTPS or hostless URL — did:web trusts TLS,
-    so there is nothing an http:// identifier could safely mean — and for a
-    URL carrying userinfo, which a did:web authority must not contain (and
-    which would otherwise leak a ``user:password@`` credential into the DID
-    embedded in every issued credential).
+    Raises ValueError for a non-HTTPS, hostless or bad-port URL — did:web
+    trusts TLS, so there is nothing an http:// identifier could safely mean —
+    and for any URL carrying an ``@`` (or ``%40``) *anywhere*, which would
+    otherwise leak a ``user:password@`` credential into the DID embedded in
+    every issued credential. No rejection quotes the URL back.
+
+    The ``@`` rule is deliberately blunt: it also refuses an innocent
+    ``https://host/@name/`` base, because once urlsplit has parsed the URL that
+    shape is indistinguishable from one hiding a credential (see below).
     """
     from urllib.parse import quote, urlsplit
     parts = urlsplit(url)
-    # Userinfo first: the two checks below quote the URL in their messages, and
-    # a CLI prints those to stdout (and into the 'error' field of --json), so
-    # 'http://user:password@host/' used to publish its own password on the way
-    # out. What they quote is redacted too, so the guarantee no longer depends
-    # on this ordering surviving a future edit.
-    if '@' in parts.netloc:
-        raise ValueError('did:web URL must not contain userinfo (user:pass@)')
-    safe = parts._replace(netloc=parts.netloc.rpartition('@')[2]).geturl()
+    # None of the messages below quotes the URL. The input is publish_url,
+    # which may carry a credential, and a CLI prints these to stdout and into
+    # the 'error' field of --json: a check that exists to keep a password out
+    # of the DID must not publish it on the way out.
+    #
+    # The whole URL is searched, not the userinfo component, because urlsplit
+    # only reports userinfo when the '@' sits inside the authority. A password
+    # holding a '/', '?' or '#' pushes the marker into the path, query or
+    # fragment, and the credential then resurfaces either through parts.port —
+    # whose ValueError quotes the offending value verbatim — or, when what
+    # precedes the ':' parses as a port, percent-encoded straight into the
+    # returned DID ('https://user:12345/x@host/' used to yield
+    # 'did:web:user%3A12345:x%40host'). A did:web base URL has no legitimate
+    # use for an '@' anywhere, so the blunt rule costs nothing real.
+    if '@' in url or '%40' in url.lower():
+        raise ValueError('did:web URL must not contain "@" (userinfo)')
+    if not parts.netloc:
+        # No '//' at all, so urlsplit read everything before the first ':' as
+        # the scheme — on a schemeless 'user:password@host/' that "scheme" IS
+        # the username, which is the other reason nothing here is quoted.
+        raise ValueError('did:web requires an https:// URL with a host')
     if parts.scheme != 'https':
-        raise ValueError('did:web requires an https URL, got %r' % (safe,))
+        raise ValueError('did:web requires an https URL')
+    try:
+        port = parts.port
+    except ValueError:
+        # urllib's own message quotes the value it could not parse, which on a
+        # 'https://user:secret/x@host/' is the password.
+        raise ValueError('did:web URL has an invalid port') from None
     if not parts.hostname:
-        raise ValueError('URL %r has no host' % (safe,))
+        raise ValueError('did:web URL has no host')
     authority = parts.hostname
-    if parts.port is not None:
-        authority += ':%d' % parts.port
+    if port is not None:
+        authority += ':%d' % port
     pieces = [quote(authority, safe='')]
     pieces += [quote(seg, safe='') for seg in parts.path.split('/') if seg]
     return 'did:web:' + ':'.join(pieces)

tests/test_cli_smoke.py

@@ -3,6 +3,8 @@
 import sys
 from unittest.mock import patch
 
+import pytest
+
 
 # ── openbadges-init ─────────────────────────────────────────────────────────────
 
@@ -508,13 +510,18 @@ def test_publish_ob2_missing_badge_key_exits_cleanly(tmp_path):
     assert 'missing required config key' in str(exc.value)
 
 
-def test_publish_never_leaks_publish_url_userinfo(tmp_path, capsys):
-    # End-to-end guarantee behind the confparser check: with a
-    # 'https://user:password@host/' publish_url, no -V publishes anything and
-    # no byte of the password reaches stdout/stderr or the output tree. Before
-    # the check, publish printed the URL on success and urljoin'd it into the
-    # ids of organization.json / badge.json / key.json.
-    import pytest
+@pytest.mark.parametrize('publish_url', [
+    'https://user:hunter2-zzz@example.com/issuer/',   # userinfo in the authority
+    'https://user:hunter2-zzz/x@example.com/issuer/',  # '/' pushes '@' out of it
+    'https://user:12345/x@example.com/issuer/',        # numeric: encoded into the DID
+])
+def test_publish_never_leaks_publish_url_userinfo(tmp_path, capsys, publish_url):
+    # End-to-end guarantee behind the confparser check: with a credential in
+    # publish_url, no -V publishes anything and no byte of the password reaches
+    # stdout/stderr or the output tree. Before the check, publish printed the
+    # URL on success and urljoin'd it into the ids of organization.json /
+    # badge.json / key.json. The last two forms are the bypass: urlsplit only
+    # reports userinfo when the '@' is inside the authority.
     from pathlib import Path
     from openbadgeslib import openbadges_publish
     tests_dir = Path(__file__).parent
@@ -523,7 +530,7 @@ def test_publish_never_leaks_publish_url_userinfo(tmp_path, capsys):
     cfg.write_text('\n'.join([
         '[paths]', 'base = %s' % tmp_path,
         '[issuer]', 'name = I', 'url = https://example.com',
-        'publish_url = https://user:%s@example.com/issuer/' % password,
+        'publish_url = %s' % publish_url,
         'revocationList = revoked.json', 'image = logo.png', 'email = i@x.example',
         '[badge_1]', 'name = B', 'description = d', 'image = badge.svg',
         'criteria = https://example.com/c', 'status_lists = revocation',
@@ -539,8 +546,9 @@ def test_publish_never_leaks_publish_url_userinfo(tmp_path, capsys):
                 openbadges_publish.main()
         assert exc.value.code == 1
         captured = capsys.readouterr()
-        assert password not in captured.out
-        assert password not in captured.err
+        for secret in (password, '12345', publish_url):
+            assert secret not in captured.out
+            assert secret not in captured.err
         assert not out.exists()
 
 

tests/test_confparser.py

@@ -166,6 +166,54 @@ def test_badge_section_url_keys_rejected(self, tmp_path, key):
         assert '[badge_1] %s' % key in str(exc.value)
         assert _PW not in str(exc.value)
 
+    @pytest.mark.parametrize('url', [
+        'https://user:12345/x@host/',        # numeric "port": encoded into the DID
+        'https://user:%s/x@host/' % _PW,     # '/' in the password moves the '@'
+        'https://user:%s?q@host/' % _PW,     # ... and so do '?' and '#'
+        'https://user:%s#f@host/' % _PW,
+        'https://user%%40host/badges/',      # '@' written as %40 ('%%' = literal %)
+        'https://host/x?u=a@b',              # '@' in the query
+    ])
+    def test_at_sign_outside_the_authority_rejected(self, tmp_path, url):
+        # urlsplit only reports userinfo when the '@' sits inside the
+        # authority, so checking netloc alone left a bypass: these all loaded
+        # cleanly and then leaked — 'https://user:12345/x@host/' became the
+        # issuer id 'did:web:user%3A12345:x%40host', carried by every signed
+        # credential, and the non-numeric ones surfaced the password through
+        # urllib's own "Port could not be cast..." message.
+        from openbadgeslib.errors import ConfigError
+        path = _write(
+            tmp_path,
+            '[paths]\nbase = .\n\n[issuer]\nname = x\npublish_url = %s\n' % url)
+        with pytest.raises(ConfigError) as exc:
+            ConfParser(path).read_conf()
+        assert _PW not in str(exc.value)
+        assert '12345' not in str(exc.value)
+
+    def test_at_sign_in_path_rejected_even_when_innocent(self, tmp_path):
+        # Deliberate false positive, documented on reject_url_userinfo: once
+        # parsed, 'https://host/@name/' is the same shape as one hiding a
+        # credential, so the blunt rule wins over the rare legitimate use.
+        from openbadgeslib.errors import ConfigError
+        path = _write(
+            tmp_path,
+            '[paths]\nbase = .\n\n[issuer]\nname = x\n'
+            'publish_url = https://issuer.example/@name/\n')
+        with pytest.raises(ConfigError):
+            ConfParser(path).read_conf()
+
+    def test_email_and_paths_with_at_sign_still_accepted(self, tmp_path):
+        # A value with no authority is not a dereferenceable URL: an address or
+        # a local path may hold an '@' and must keep loading.
+        path = _write(
+            tmp_path,
+            '[paths]\nbase = .\n\n[issuer]\nname = x\n'
+            'email = issuer_mail@issuer.badge\n\n'
+            '[badge_1]\nmail = ${paths:base}/@inbox/badge_1.txt\n')
+        conf = ConfParser(path).read_conf()
+        assert conf['issuer']['email'] == 'issuer_mail@issuer.badge'
+        assert conf['badge_1']['mail'].endswith('/@inbox/badge_1.txt')
+
     def test_interpolated_userinfo_rejected(self, tmp_path):
         # The check runs after ${...} resolution, so a credential cannot be
         # smuggled in through a reference to an unchecked section.

tests/test_ob3_did_doc.py

@@ -51,17 +51,51 @@ def test_rejects_userinfo(self, url):
         'http://user:hunter2-zzz@example.com/',   # fails the scheme check too
         'https://user:hunter2-zzz@',              # fails the host check too
         'ftp://user:hunter2-zzz@example.com/',
+        'user:hunter2-zzz@example.com/',          # schemeless: "scheme" == user
     ])
     def test_no_message_echoes_the_password(self, url):
-        # The userinfo check must run *before* the scheme and host checks,
-        # which quote the URL: a CLI prints the ValueError to stdout (and into
-        # the 'error' field of --json), so a password reaching those messages
-        # would be published by the very error meant to reject it.
+        # The userinfo check must run *before* the scheme and host checks: a
+        # CLI prints the ValueError to stdout (and into the 'error' field of
+        # --json), so a password reaching those messages would be published by
+        # the very error meant to reject it.
         with pytest.raises(ValueError) as exc:
             did_web_from_url(url)
         assert 'hunter2-zzz' not in str(exc.value)
         assert 'hunter2-zzz' not in repr(exc.value)
 
+    @pytest.mark.parametrize('url,secret', [
+        # urlsplit only reports userinfo when the '@' sits inside the
+        # authority. A password holding a '/', '?' or '#' pushes it into the
+        # path, and the credential then resurfaced two ways: percent-encoded
+        # into the returned DID when what precedes the ':' parses as a port
+        # (this yielded 'did:web:user%3A12345:x%40host'), or through
+        # parts.port's own ValueError, which quotes the value verbatim.
+        ('https://user:12345/x@host/', '12345'),
+        ('https://user:hunter2-zzz/x@host/', 'hunter2-zzz'),
+        ('https://user:hunter2-zzz?q@host/', 'hunter2-zzz'),
+        ('https://user:hunter2-zzz#f@host/', 'hunter2-zzz'),
+        ('https://user%40host/badges/', 'user%40host'),      # '@' written %40
+    ])
+    def test_at_sign_outside_the_authority_is_refused(self, url, secret):
+        with pytest.raises(ValueError) as exc:
+            did_web_from_url(url)
+        assert secret not in str(exc.value)
+        assert secret not in repr(exc.value)
+
+    def test_invalid_port_does_not_quote_the_value(self):
+        # urllib's own "Port could not be cast to integer value as 'x'" quotes
+        # what it could not parse — which on a 'user:password/...' URL is the
+        # password. The parse must be wrapped and re-raised without it.
+        with pytest.raises(ValueError) as exc:
+            did_web_from_url('https://host:hunter2-zzz/badges/')
+        assert 'hunter2-zzz' not in str(exc.value)
+
+    def test_at_sign_in_path_is_refused_even_when_innocent(self):
+        # Deliberate false positive, documented on the function: after parsing,
+        # 'https://host/@name/' is the same shape as one hiding a credential.
+        with pytest.raises(ValueError):
+            did_web_from_url('https://example.com/@name/')
+
     @pytest.mark.parametrize('url,doc_url', [
         ('https://example.com', 'https://example.com/.well-known/did.json'),
         ('https://example.com/badges/', 'https://example.com/badges/did.json'),