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

Extend safeHref #58

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Extend safeHref #58

wants to merge 12 commits into from

Conversation

colinlohner
Copy link
Contributor

@colinlohner colinlohner commented Mar 26, 2020

An update to the safeHref functionality to better support our users

  • Removed data: protocol from safe protocols to further protect users
  • Added ability to pass custom "safe" protocols to the configureSafeHref method
  • Added ability to allow custom protocols on a component usage level allowProtocols
  • Fixed a situation where if whitespace led a url you could bypass protocol safety
  • Added some basic testing around the functionality of safeHref

@colinlohner colinlohner self-assigned this Mar 26, 2020
src/box.tsx Outdated Show resolved Hide resolved
src/utils/safeHref.ts Outdated Show resolved Hide resolved
src/utils/safeHref.ts Outdated Show resolved Hide resolved

const Box: BoxComponent = ({ is = 'div', innerRef, children, allowUnsafeHref, ...props }) => {
const Box: BoxComponent = ({ is = 'div', innerRef, children, allowUnsafeHref, allowProtocols, ...props }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add these to the PropTypes as well ( allowUnsafeHref and allowProtocols)

src/utils/safeHref.ts Outdated Show resolved Hide resolved
src/utils/safeHref.ts Outdated Show resolved Hide resolved
src/utils/safeHref.ts Outdated Show resolved Hide resolved
src/utils/safeHref.ts Outdated Show resolved Hide resolved
Copy link

@ucarion ucarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some thoughts! Overall LGTM. :)

@@ -22,7 +22,16 @@ const Box: BoxComponent = ({ is = 'div', innerRef, children, allowUnsafeHref, ..
*/
const safeHrefEnabled = (typeof allowUnsafeHref === 'boolean' ? !allowUnsafeHref : getUseSafeHref()) && is === 'a' && parsedProps.href
if (safeHrefEnabled) {
const {safeHref, safeRel} = extractAnchorProps(parsedProps.href, parsedProps.rel)
const hrefData:HrefData = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: What's the motivation for explicitly marking the type here? What happens if you remove :HrefData?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we opt to keep this as-is, I would suggest a space between hrefData: and HrefData.

/**
* - Find protocol of URL or set to 'relative'
* - Find origin of URL
* - Determine if sameOrigin
* - Determine if protocol of URL is safe
*/
const protocolResult = url.match(PROTOCOL_REGEX)
const originResult = url.match(ORIGIN_REGEX)
const cleanedUrl = url.trim()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might we worth putting a comment motivating why this is a sufficient "cleanup" step? For instance a link to:

https://html.spec.whatwg.org/multipage/links.html#links-created-by-a-and-area-elements

I can confirm that this is a sufficient cleanup, at least in Node:

> ["\u0009", "\u000A", "\u000C", "\u000D", "\u0020"].map(s => s.trim())
[ '', '', '', '', '' ]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use url.parse (https://nodejs.org/api/url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost) and then grab the pieces that you need.

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.

4 participants