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

Add in same-origin check for ExpressRoute #158

Merged
merged 3 commits into from
Jan 20, 2017

Conversation

jeffposnick
Copy link
Contributor

@gauntface
Copy link

What happens when a user does pass in a different origin?

@jeffposnick
Copy link
Contributor Author

I added in an explicit check to make sure that the path starts with '/', and I'm using ErrorFactory to throw a custom error if it doesn't.

I also made a slight tweak to the base ErrorFactory class to have it strip out repeated whitespace in the error message before throwing. This makes it easier to define multi-line error messages using ES2015 string literal backticks, while still respecting our line length linting, and not having extra newlines and whitespace make it through to the message itself. See https://github.com/GoogleChrome/sw-helpers/pull/158/files#diff-cad42d520bcc18461d07406f91d64442R19 for an example of what I mean about the nicer syntax.

If you'd rather I not tweak ErrorFactory like that, I can back it out of this PR.

@gauntface
Copy link

ErrorFactory change looks good.

With the slash, is there any chance someone would go: new ExpressRoute('site-section/:sectionId') I could picture this being intended as a path relative to the service worker location.

@jeffposnick
Copy link
Contributor Author

I poked around the path-to-regexp library that's used under the hood. It would be okay with paths that don't start with '/'. Moreover, path-to-regexp would be okay with full URLs that include origins, and would convert them into a reasonable-looking RegExp.

The reason not to support anything other than paths that start with '/' is that we're billing this library as being "Express"-style routing, and from what I can tell from the Express docs, it always requires routes to start with '/'.

So it's a somewhat artificial requirement meant to enforce Express's restrictions. Of course, if we didn't behave like Express then this module would undergo an existential crisis, so I'm in favor of maintaining that requirement.

@gauntface
Copy link

I'm a tad confused why your build is unhappy given no changes in this should have upset - trying to fix travis....

@jeffposnick
Copy link
Contributor Author

Looks like it's cleared up now, but yes, that is odd...

I'm going to merge.

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.

3 participants