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

Running on Heroku with one-click deploy #320

Open
4 tasks
Lewiscowles1986 opened this issue Feb 10, 2021 · 11 comments
Open
4 tasks

Running on Heroku with one-click deploy #320

Lewiscowles1986 opened this issue Feb 10, 2021 · 11 comments

Comments

@Lewiscowles1986
Copy link

Lewiscowles1986 commented Feb 10, 2021

In order to run on Heroku with one-click deploy, I setup a fork https://github.com/Lewiscowles1986/cors-anywhere

If Rob wants it, I'm happy to upstream and I kept each branch alive so I can rebase on their repo. I made some other choices too, such as renaming some bits and enabling CI.

This issue is here as "documentation" of how to setup your own cors anywhere using Heroku.

Summary of discussion of changes

  • Supply app.json to deploy to Heroku.
  • (author preference) More inclusive terminology.
  • Backwards compatibility code to let existing users use non-inclusive terminology, while issuing warning to console.
  • suggested improvement. Specify repository, so source code updates can ship to heroku and heroku local-machine git can be used.
@Rob--W
Copy link
Owner

Rob--W commented Feb 10, 2021

To minimize backwards-incompatible breakage and confusion, I'm not going to accept the "black"/"white"->"allow"/"block" change.

Documentation for the Heroku one-click deploy feature is at https://devcenter.heroku.com/articles/heroku-button

A completely open proxy is against Heroku's Acceptable Use policy. The current version of your PR makes the proxy open by default. Is it possible to require explicit action from developers, so that whoever that wants to use the button knows that they have to change the configuration?

@Lewiscowles1986
Copy link
Author

It's written on the screen and in your docs. I've added to only have localhost in the allowlist and 1 1 in the rate limit. This seemed to block unwanted traffic.

@bobpaul
Copy link

bobpaul commented Mar 17, 2021

Regarding backwards compatibility for black/block type change, might I suggest supporting "blackList" and "whiteList" as aliases? Eg: If both CORSANYWHERE_BLACKLIST and CORSANYWHERE_BLOCKLIST throw an error, but if only one is defined use that one. Update documentation to point to BLOCKLIST and either do or don't document the alias.

@Lewiscowles1986
Copy link
Author

Lewiscowles1986 commented Mar 17, 2021

Right there is a branch at https://github.com/Lewiscowles1986/cors-anywhere/tree/chore/backwards-compat-uninclusive-terms which does this, and console warns on use of the deprecated properties.

Logic is:

  • It gets both Inclusive and mainline ENV.
  • For now it passes two properties not set in lib/cors-anywhere.js options via server.js
  • If the old properties are set, and the more inclusive properties are not it warns via the console and transparently copies them into the needed place.
  • This all means it lives on the outer-shell of the application, and duplicate logic does not need to go in too many places.

Perhaps this removes the rejection of the rename.

I'm just not happy using those terms, but I understand the need to roadmap changes. I Have opted not to document the alias. New users should not really be choosing to deliberately use discriminatory language. It is also not part of the Heroku one-click as that is similarly a greenfield, rather than for existing users.

Lewiscowles1986 added a commit to Lewiscowles1986/cors-anywhere that referenced this issue Mar 17, 2021
RE: comments on [Issue Rob--W#320][1]

Make a path to move to more progressive terminology.

[1]: https://github.com/Rob--W/cors-anywhere/issues/320#issuecomment-801445034
Lewiscowles1986 added a commit to Lewiscowles1986/cors-anywhere that referenced this issue Mar 17, 2021
RE: comments on [Issue Rob--W#320][1]

Make a path to move to more progressive terminology.

[1]: https://github.com/Rob--W/cors-anywhere/issues/320#issuecomment-801445034
@bobpaul
Copy link

bobpaul commented Mar 17, 2021

@Lewiscowles1986, after I 1-click deployed from your repo's, I got an empty repo when I heroku git:clone the created app. I found a work around here, but I think if you included the repository property in app.json, I would not have had to do that.

@Lewiscowles1986
Copy link
Author

Hey Bob,

Sorry to hear you've had that difficulty. Could you share some more information about your setup? I Just did the same thing both from my main branch and the branch I linked here, and both are working for me in Chrome, Firefox, Edge and Safari (all latest) across Windows, Linux and OSx. Did you need to login to Heroku first?

I Do know that deploying from app.json does not automatically setup deploy from git commits. I think if I point it at my repository, then there would be more of a problem merging as it would be hosted here, but point to a potentially out-of-date branch, rather than official cors-anywhere.

@bobpaul
Copy link

bobpaul commented Mar 18, 2021

The deploy worked fine. I clicked the button and got a screen asking for the environment variables. I was able to deploy the app to heroku as "myApp"

After it deployed and was running, I used the heroku cli and did heroku git:clone -a {myApp} and it gave me an empty repo. It's been a while since I deployed a click-to-deploy, but I thought they usually were attached to the original repo in some way for re-deploy and updates.

@Lewiscowles1986
Copy link
Author

As mentioned @bobpaul once merged upstream this could be a thing. Right now it would make me an unofficial maintainer of something I only encountered when helping a friend. I saw some problems (for us), and came up with a solution to fix them, and then offered what we had. It sounds like a great suggestion for a next step. I Think if we could get this merged first, then it would be easier to make a tiny patch referencing this comment on this issue. I'll add it to the top as an action item.

@Rob--W
Copy link
Owner

Rob--W commented Mar 22, 2021

I'm planning to prepare a new release soonish (but not urgent), and am considering to accept the requested functionality.

The branch is doing too many unrelated things. Could you split it in separate pull requests so that they can be reviewed and merged independently? Please keep the number of unnecessary changes to a minimum (and have meaningful commits; otherwise I would need to squash them all into one commit). I suggest the following order (the Heroku one-click deploy last because it depends on the renamed variable).

  1. PR to rename "blacklist" to "blocklist" and "whitelist" to "allowlist" - with proper capitalizations (not just a blind search-and-replace like you've done right now). Do not print a deprecation warning, I don't want to force unnecessary warnings on existing users.
  2. PR to add the Heroku one-click deploy button.

I'm not sure what you mean by "suggested improvement. Specify repository, so source code updates can ship to heroku and heroku local-machine git can be used.". Why is that part of your branch? FYI: My heroku demo runs a fork of the master branch (with patches on top to enforce restrictions to comply with the policy). That use case should not be endangered by the proposed changes.

Lewiscowles1986 added a commit to Lewiscowles1986/cors-anywhere that referenced this issue Mar 23, 2021
RE: comments on [Issue Rob--W#320][1]

Make a path to move to more progressive terminology.

[1]: https://github.com/Rob--W/cors-anywhere/issues/320#issuecomment-801445034
@pierre1590
Copy link

Hi, I have the following code:
const res = await fetch(https://cors-anywhere.herokuapp.com/${url}`);
const data = await res.json();`

how can I fix the 403 (Forbidden) error?

@Rob--W
Copy link
Owner

Rob--W commented Mar 30, 2021

@Lewiscowles1986 I just noticed that you added a commit there, but there is no open pull request to review. If your patches are ready for review, please open a PR.

@pierre1590 Your question is unrelated to this issue, please don't post unrelated comments on random issues. For your specific issue, see the pinned issue for the explanation.

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

No branches or pull requests

4 participants