Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't allow ' in path #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Don't allow ' in path #55

wants to merge 1 commit into from

Conversation

alexpusch
Copy link

failing test case: background: url('http://example.com/pic.jpg');
resulted in http://example.com/pic.jpg'

fixed #54

@sindresorhus
Copy link
Contributor

@alexpusch
Copy link
Author

well than... So the test cast I've mentioned is ambiguous. One can make the argument that it would be better to not support ' as part of the path. Many use cases of this module might encounter an enclosing ', but much fewer will encounter them as an intended charterer of the url

What do you think? If you'll decide to stick to the specs - also a valid argument, close this issue.

@gajus
Copy link

gajus commented Aug 26, 2019

I think this could be addressed in get-urls instead of here: adding an option to cut off anything beginning with ' or ". That is what I did:

// @flow

import urlRegex from 'url-regex';
import {
  uniq,
} from 'lodash';
import normalizeUrl from './normalizeUrl';

const extractUrl = (subject: string): string => {
  const matchedUrls = subject.match(urlRegex()) || [];

  const normalizedUrls = uniq(matchedUrls.map((matchedUrl) => {
    return normalizeUrl(matchedUrl.split(/['"]/)[0]);
  }));

  if (normalizedUrls.length === 0) {
    throw new Error('URL not found.');
  }

  if (normalizedUrls.length > 1) {
    throw new Error('Found multiple URLs. Input must contain at most one URL.');
  }

  return normalizedUrls[0];
};

@niftylettuce
Copy link
Collaborator

niftylettuce commented Aug 15, 2020

Feel free to submit a PR to https://github.com/niftylettuce/url-regex-safe. See comment below

@niftylettuce
Copy link
Collaborator

I have already done this for you, and added an apostrophe option to url-regex-safe, which is false by default (which matches the PR behavior you've submitted). Please try url-safe-regex v0.0.4+ at https://github.com/niftylettuce/url-safe-regex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

urls that are enclosed with ' instead of " are not matched correctly
4 participants