Links to protocol-relative URLs are not marked rel=nofollow about rehype-external-links HOT 9 CLOSED

Heya! Some thoughts:

• Perhaps the “current” (assumed) protocol might have to be passed, defaulting to say http? This might be useful to have because we have a protocols option.
• Would it be needed to support this on, say, an ftp, gemini or something? Not all protocols need slashes (e.g., tel, mailto), what to do there?
• The code here (different project) can properly find if a dangerous protocol is used, assuming the page will be on a safe protocol instead. Something like that might be useful here but is not enough, for your slashes example

Alternatively, perhaps this project would be improved if we a) pass the URL where the page will hosted, b) run new URL(a.href, baseUrl), c) check if the domains are equal or different.
This has the downside of potentially broken URLs either crashing or alternatively always marked as external (?), but the upside of full URLs with protocols or slashes that go to my own domain being marked as safe

from rehype-external-links.

I wish new URL didn't require a base URL to be a useful, robust URL parser. :( Requiring that to be set by the caller to get this check would probably result in nobody getting this check.

Probably the safest thing to do is assume that any href starting with // is an external URL (RFC 3986 § 3; When authority is not present, the path cannot begin with two slash characters ("//").); I'd argue there's no reasonable use case for treating a URL with an authority in it as "relative" for the purposes of this library.

Given that protocol-relative URLs are frowned up on in web dev, I think the most likely use of a protocol-relative URL in the wild is someone who is trying to work around this package's checks (and I say this as someone who reported this bug because I was able to work around it in the wild).

from rehype-external-links.

Requiring that to be set by the caller to get this check would probably result in nobody getting this check.

This sounds a bit “adamant”. Why so certain? We could even require it and throw if it isn’t configured. I think it’s quite an improvement to parse “full” URLs as well.

Some plugins need it too, and they currently support:

const file = await read('example.md')

// Define where the generated file will be available.
file.data.meta = {
  origin: 'https://planets.com/',
  pathname: '/neptune/'
}

await unified()
  ...

I'd argue there's no reasonable use case for treating a URL with an authority in it as "relative" for the purposes of this library.

Yeah, agreed. Interested in making a PR?

from rehype-external-links.

Yeah, agreed. Interested in making a PR?

Yeah, can do that. May be a few days.

This sounds a bit “adamant”. Why so certain?

Sorry it came off that way, I tend to be too terse. I didn't think about requiring the base URL to be set, I sort of assumed it'd be optional; although I'm sure there's current consumers for whom adding that wouldn't make sense for their use case.

My plan is to send a PR that just checks for // at the start of the URL and treats it the same as if the is-absolute-url library returns true; it doesn't change too much and adds a mitigation.

from rehype-external-links.

Sorry it came off that way

No worries!

although I'm sure there's current consumers for whom adding that wouldn't make sense for their use case.

One alternative, that might work: default to an origin of localhost:8000. It would still classify URLs to the actual page as external, but well, that’s the current state.
And I don’t imagine wrongly not marking localhost:8000 as external is a (security) problem?

it doesn't change too much and adds a mitigation.

Appreciate it!

from rehype-external-links.

Hi! This was marked as ready to be worked on! Note that while this is ready to be worked on, nothing is said about priority: it may take a while for this to be solved.

Is this something you can and want to work on?

Team: please use the area/* (to describe the scope of the change), platform/* (if this is related to a specific one), and semver/* and type/* labels to annotate this. If this is first-timers friendly, add good first issue and if this could use help, add help wanted.

from rehype-external-links.

Hi! This was marked as ready to be worked on! Note that while this is ready to be worked on, nothing is said about priority: it may take a while for this to be solved.

Is this something you can and want to work on?

Team: please use the area/* (to describe the scope of the change), platform/* (if this is related to a specific one), and semver/* and type/* labels to annotate this. If this is first-timers friendly, add good first issue and if this could use help, add help wanted.

from rehype-external-links.

@iliana Still interested in/available to work on this? Can I help you?

from rehype-external-links.

thanks iliana, released in 2.0.1!

from rehype-external-links.