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 new 'ACKEE_AUTO_FQDN_ORIGIN' method of adding CORS Headers #271

Merged
merged 1 commit into from
Feb 26, 2022

Conversation

tkw1536
Copy link
Contributor

@tkw1536 tkw1536 commented Jun 28, 2021

Previously there were two builtin methods of adding CORS headers to ACKEE:

  • Set a wildcard header (ACKEE_ALLOW_ORIGIN=*); or
  • Set a concrete list of domains (ACKEE_ALLOW_ORIGIN=domain1.example.com,domain2.example.com)
    However these become very annoying (either insecure or tedious) to configure when working with more than a couple of domains.

This commit proposes adding a new method 'ACKEE_AUTO_FQDN_ORIGIN' to figure out which domains to add CORS Headers for.
Instead of hard-coding the domains, this mode instead determines them from the list of tracking domains.
Concretely, each domain in the domain list that has a fully qualified domain name as a title is treated as a valid domain to add a CORS header for.

The implementation is achieved by means of a new 'domainFqNames.js' utility, which uses the 'is-valid-domain' npm package to filter the list of domains.
Additionally the 'findMatchingOrigin.js' utility is modified to make use of the above.
A second equivalent implementation is added to 'serverless.js'.

This commit also adds a test for the new functionality, although this does not test the serverless implementation.

Previously there were two builtin methods of adding CORS headers to
ACKEE: One could either set a wildcard header (`ACKEE_ALLOW_ORIGIN=*`)
or set a concrete list of domains
(`ACKEE_ALLOW_ORIGIN=domain1.example.com,domain2.example.com`). However
these became very annoying to configure when working with more than a
couple of domains.

This commit proposes adding a new method 'ACKEE_AUTO_FQDN_ORIGIN' to
figure out which domains to add CORS Headers for. Instead of hard-coding
the domains, this mode instead determines them from the list of tracking domains.
Conretely, each domain in the domain list that has a fully qualified
domain name as a title is treated as a valid domain to add a CORS header for.

The implementation is achieved by means of a new 'domainFqNames.js'
utility, which uses the 'is-valid-domain' npm package to filter the list
of domains. Additionally the 'findMatchingOrigin.js' utility is modified
to make use of the above. A second equivalent implementation is added
to 'serverless.js'.

This commit also adds a test for the new functionality, although this
does not test the serverless implementation.
@vercel
Copy link

vercel bot commented Jun 28, 2021

Someone is attempting to deploy a commit to a Personal Account owned by @electerious on Vercel.

@electerious first needs to authorize it.

@BetaHuhn
Copy link
Contributor

As I already mentioned in #159 I would appreciate such a feature as it's very cumbersome to manually ssh into my server and add the domain to the env variable ACKEE_ALLOW_ORIGIN every time I want to add a new site.

I also think the way this PR implements it is great as it works around the limitation mentioned in #159 (comment).

@electerious
Copy link
Owner

This looks great! I just don't have the time to review and test the PR, but I will keep it on my list when I'm back working on Ackee. Thanks for the work!

@tkw1536
Copy link
Contributor Author

tkw1536 commented Aug 31, 2021

Any updates on merging or not merging this?

@4ndv
Copy link

4ndv commented Feb 14, 2022

This would be very nice to have, currently I'm entering all the cors domains by hand, which is really annoying

@tkw1536
Copy link
Contributor Author

tkw1536 commented Feb 14, 2022

Same, @electerious anything I can do to help get this merged?

@electerious electerious changed the base branch from master to develop February 26, 2022 12:51
electerious added a commit that referenced this pull request Feb 26, 2022
@electerious electerious merged commit 7a64124 into electerious:develop Feb 26, 2022
@electerious
Copy link
Owner

electerious commented Feb 26, 2022

Merged! Thanks a lot for your work. The option is now called ACKEE_AUTO_ORIGIN and a part of the develop branch. There's however a blocker for serverless installations which is why I can't completely test and release the implementation: #307 Help is welcome!

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