security(config): refuse any '@' in a published URL, not just userinfo - #256
Merged
Conversation
The guard added in 0954e9d could be bypassed. It asked urlsplit for the authority and looked for an '@' there, but 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 both layers then waved the value through. Two ways the credential resurfaced, verified end to end against 0954e9d: publish_url = https://user:12345/x@host/ -> config accepted, issuer id = did:web:user%3A12345:x%40host (the credential percent-encoded into the DID that travels in `issuer` on every signed credential — exactly what the guard existed to prevent) publish_url = https://user:secret/x@host/ -> config accepted, then ValueError: Port could not be cast to integer value as 'secret' (urllib raises that one, so the redaction added in 0954e9d — which only covers our own messages — never touched it; it reaches stdout and the --json `error` field) A published identifier has no legitimate use for an '@', so both layers now refuse one anywhere in a URL-shaped value, '%40' included, instead of parsing out a userinfo component that a malformed URL can hide. A value with no authority is left alone, so [issuer] email and local paths keep their '@'. did_web_from_url additionally wraps the parts.port parse (its ValueError quotes the value) and no longer quotes the URL in any message — not even redacted, since on a schemeless 'user:password@host/' urlsplit reads the username as the scheme. BREAKING (narrow): a publish_url whose path contains an '@', such as https://host/@name/, is now refused. Once parsed, that shape is indistinguishable from one hiding a credential; the blunt rule wins. Tests cover the five bypass shapes at both layers plus end to end through publish -V 1/2/3, assert no message quotes the value, and pin that an address or local path with an '@' still loads. Closes #255.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #255.
The guard from #254 could be bypassed: it looked for an
@inurlsplit(...).netloc, but urlsplit only reports userinfo when the@sitsinside the authority. A password holding a
/,?or#pushes the markerinto the path, and both layers waved the value through.
Verified end to end against
0954e9dbefore the fix:publish_urlhttps://user:12345/x@host/did:web:user%3A12345:x%40hosthttps://user:secret/x@host/ValueError: Port could not be cast to integer value as 'secret'So the credential went into the DID carried by every signed credential, or
into an error message on stdout and the
--jsonerrorfield. That secondone is raised by urllib, so the redaction #254 added — which only covers our own
messages — never reached it.
The rule
A published identifier has no legitimate use for an
@, so both layers nowrefuse one anywhere in a URL-shaped value (
%40included) instead ofparsing out a userinfo component a malformed URL can hide. A value with no
authority is left alone, so
[issuer] emailand local paths keep their@.did_web_from_urlalso wraps theparts.portparse and quotes nothing at all —not even a redacted copy, since on a schemeless
user:password@host/urlsplitreads the username as the scheme.
BREAKING (narrow): a
publish_urlwhose path contains an@, e.g.https://host/@name/, is now refused. Once parsed it is indistinguishable fromone hiding a credential.
Tests
17 new assertions: the five bypass shapes at both layers, the invalid-port
message, end to end through
publish -V 1/2/3, and a pin that an address orlocal path with an
@still loads. All 17 fail on0954e9d.flake8clean,mypyclean, 1326 tests pass.