Skip to content
This repository has been archived by the owner on Aug 17, 2021. It is now read-only.

Allow requests from any domain in the development environment. #71

Merged

Conversation

marcosvafilho
Copy link
Contributor

@marcosvafilho marcosvafilho commented Oct 14, 2020

This PR allows requests from any origin in the development environment, so you don't need to waste time figuring out exactly which domain and port your browser or api client is using to make requests while testing/debugging.

It still validates if the request method/verb is explicitly allowed. Ideally we would want our development environment to behave as close as possible to production, specially while debugging, being the origin domain the only real constraint to solve.

closes #70

@marcosvafilho
Copy link
Contributor Author

Hey @MauricioRobayo ,

Thank you so much for quickly answering me about the possibility to whitelist any and all domains while running in development. Game changer for me!

I tried your proposed solution but for some reason it makes any and all requests point to the very first proxy, ignoring the others.

Using the default examples, both /ipinfo and /github insist on pointing to /weather when directly using return true inside the filter function:

  const filter: Filter = (pathname: string, req: Request) => {
+   if (process.env.NODE_ENV === 'development') {
+     return true
+   }
    if (typeof req.headers.origin === 'string') {
      return (
        pathname.startsWith(route) &&
        allowedMethods.includes(req.method) &&
        [...globalAllowedDomains, ...allowedDomains].includes(
          req.headers.origin
        )
      );
    }
    return false;
  };

In order to avoid that, I needed to include the pathname.startsWith(route) check, which somehow makes pathnames work as intended (sorry, not a node/express guy to understand exactly why).

Since I was already including that and the domain validation was the only real constraint, I decided to also include allowedMethods.includes(req.method) in order to keep the request method validation working the same as in production:

  const filter: Filter = (pathname: string, req: Request) => {
+   if (process.env.NODE_ENV === 'development') {
+     return pathname.startsWith(route) && allowedMethods.includes(req.method);
+   }
    if (typeof req.headers.origin === 'string') {
      return (
        pathname.startsWith(route) &&
        allowedMethods.includes(req.method) &&
        [...globalAllowedDomains, ...allowedDomains].includes(
          req.headers.origin
        )
      );
    }
    return false;
  };

Please let me know if you have any tips on making this code more elegant and aligned to what you want to see in your project.

Best,
Marcos.

@MauricioRobayo
Copy link
Owner

This looks great @marcosvafilho , thank you for making sure it is working as expected, your implementation looks great!

I forgot to mention it before, but it would be great if the README.md included this change on the Test it on development section, we could remove step 2 (Allow the domains on the proxy) and mention in step 1 that all domains are allowed while on development mode, what do you think?

It's OK if you don't have the time to do that update, although it would be great to honor your contribution, but if you don't have time it's fine, I understand, just let me know either way 👍 .

@marcosvafilho
Copy link
Contributor Author

I'll do it later today, thanks @MauricioRobayo 👍

@marcosvafilho
Copy link
Contributor Author

It's done @MauricioRobayo , please review it when you have the time. And thank you for the opportunity to contribute to my first ever open source project!

@MauricioRobayo MauricioRobayo merged commit 6b7aff7 into MauricioRobayo:master Oct 14, 2020
@MauricioRobayo
Copy link
Owner

Thanks to YOU @marcosvafilho , this is an excellent improvement to the project!

@marcosvafilho marcosvafilho deleted the feat_any_domain_development branch October 15, 2020 04:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow any and all domains instead of specific ones for local development and tests
2 participants