Repository navigation
Properly log validation errors related to legacy IPv4 notations #498
Description
The current rev of the URL Standard specifies that the various legacy notations accepted by inet_aton for IPv4 addresses are validation errors, but rust-url silently accepts them (they're not even diagnosed via the syntax violation callback). I would like to have an option to reject these.
Examples of undesirable constructs:
# all are treated as equivalent to http://127.0.0.1/
http://127.0.1/
http://127.1/
http://0177.0.1/
http://0177.0.0.1/
http://0177.1/
http://0x7f.0.1/
http://0x7f.0.0.1/
http://0x7f.1/
http://2130706433/
http://0x7f000001/
http://017700000001/
# equivalent to http://127.0.0.8/ , not http://127.0.0.10/
http://0127.0.0.010/
See https://url.spec.whatwg.org/#concept-ipv4-parser (pay close attention to where it, and its subroutine https://url.spec.whatwg.org/#ipv4-number-parser , set the "validationErrorFlag") for gory details.
Previous discussion in #116.
Activity
dekellum commentedon May 8, 2019
I wasn't aware of this legacy edge case (thanks!), but I agree with your motivation. Seems exploitable for misuse. For example servers might have good reason to want to reject provided URLs to localhost. Do common IPv4 resolvers actually accept and resolve these examples to local ports?
If the violation callback was extended for this case, you would have the option of promoting the violation to your own error or other form of rejection? That would be a light-weight and (I think, reasonably) backward-compatible way to achieve it, while still making the implementation available in this crate. I'd use that!
zackw commentedon May 8, 2019
dekellum commentedon May 9, 2019
Indeed! I've suggested keeping it. :)
SimonSapin commentedon May 10, 2019
Aligning with the spec in terms of what inputs log a syntax violation sounds good.
zackw commentedon May 10, 2019
@dekellum re
The POSIX spec for
getaddrinfosays that "address strings using Internet standard dot notation as specified in inet_addr are valid", and that does include all of the unusual syntaxes I listed.dekellum commentedon May 10, 2019
@SimonSapin : Thanks. I think anyone implementing would be concerned about #462 Is it a reasonable time to decide that either way?
nox commentedon Jul 18, 2019
AFAIK if we want to do that we need a new variant for the syntax violation enum, so this will be a breaking change, so we want to do that for 2.0 I guess.
[-]Option to reject legacy inet_aton notation for IPv4 addresses[/-][+]Properly log validation errors related to legacy IPv4 notations[/+]dekellum commentedon Jul 18, 2019
Would discouraging exhaustive matching (in itself a breaking change) on
SyntaxViolation, and ideallyParseError, be good for 2.0, making this non-breaking when available? I can't parse the tracking in rust-lang/rust#44109 or guess what that stable MSRV would be, so likely the old fashion manual way with an unused variant would be best for now.SimonSapin commentedon Jul 19, 2019
It looks like
#[non_exhaustive]is not stable in any release so far.dekellum commentedon Jul 19, 2019
Okay, would you entertain a PR to add an unused variant and rustdoc to each of these enums?
dekellum commentedon Jul 19, 2019
Also, having looked at these error types, they are in need of some other modernizations, like dropping description() impl and replacing with a Display impls. I could do that in same PR. Of interest?
nox commentedon Jul 19, 2019
I think it would be ok adding a
doc(hidden)variant and making it extra clear in the docs that this enum is never expected to be exhaustively matched.nox commentedon Jul 19, 2019
Nah please leave that alone, no need to regress things for no reason and the implementation is trivial. You can definitely add a Display impl though.
dekellum commentedon Jul 19, 2019
IIRC, current
std::error::Errordefault impls (now deprecated)descriptionvia Display, but I get your point, thanks.SimonSapin commentedon Jul 19, 2019
descriptionreturns a&strthat borrows&self(not for example aString) so it can’t be implemented based onDisplay(only the other way around is possible).You can click on the
[src]link in https://doc.rust-lang.org/std/error/trait.Error.html to quickly see the body of default methods.Auto merge of #525 - dekellum:future-proof, r=SimonSapin
Auto merge of #525 - dekellum:future-proof, r=SimonSapin
Auto merge of servo#525 - dekellum:future-proof, r=SimonSapin