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

allowedOrigins error at server.js line 277 #850

Open
djlogan2 opened this issue Sep 3, 2022 · 9 comments
Open

allowedOrigins error at server.js line 277 #850

djlogan2 opened this issue Sep 3, 2022 · 9 comments

Comments

@djlogan2
Copy link

djlogan2 commented Sep 3, 2022

Hi guys

I believe your code for allowedOrigins has a bug. In server.js, line 277, the if statement does not actually allow for allowedOrigins to contains a regex expression as your documentation states.

The workaround is to add a line after the "new FileCollection" with: obj.allowedOrigins = /xyz/ but obviously this is not ideal, particularly since your documentation states otherwise.

@dr-dimitru
Copy link
Member

@djlogan2 thank you for reporting this.

As I remember @menelike and @s-ol authored this feature. Guys, can you check this one? Related piece of code;

@dr-dimitru
Copy link
Member

Friendly ping.
@djlogan2 have you managed to solve this one? How? Just with the workaround you mention?
@menelike @s-ol any thoughts?

@djlogan2
Copy link
Author

djlogan2 commented Nov 2, 2022

Yessir, I implemented the workaround just as I said above. Here is a real world example working in production:

export const ImportedPgnFiles = new FilesCollection({
  collectionName: "importedPgnFiles",
  allowClientCode: false, // Disallow remove files from Client
  allowedOrigins: /(.*chessclub.com.*)|(.*localhost.*)/,
  allowQueryStringCookies: true,
  onBeforeUpload(file) {
    // Allow upload files under 10MB, and only in png/jpg/jpeg formats
    if (/*file.size <= 10485760 && */ /pgn/i.test(file.extension)) {
      return true;
    }
    return "Please upload PGN file";
  },
});
ImportedPgnFiles.allowedOrigins = /(.*chessclub.com.*)|(.*localhost.*)/;

@dr-dimitru
Copy link
Member

@djlogan2 great, thank you. I feel like we can close it, unless you wish to update documentation? PRs are welcome

P.S. you're checking pgn instead of png extension

@djlogan2
Copy link
Author

djlogan2 commented Nov 2, 2022

Yes, "pgn" is "Portable Game Notation." I didn't fix the comment. I'll do that now :)

Re closing, I suppose it depends upon whether you want your users to have to add another line or use the configuration object. That's not my call. Your doc says it works, and in the code it actually sets allowedOrigin from the configuration object, just incorrectly. In my opinion, if you want users to write an extra line, changing the doc is required, but so is removing the erroneous code.

@dr-dimitru
Copy link
Member

dr-dimitru commented Nov 2, 2022

Lol, yeah, In looked at the comment, then looked at name of collection importedPgnFiles and started thinking it might be on purpose 😅

I'm open to continue discussing this one, but I need to know why you are the first one to report this. Since it wasn't reported by other developers I assume it's isolated to you. If so, we need to know what's unique about your project, and add this exception to the docs.

@djlogan2
Copy link
Author

djlogan2 commented Nov 2, 2022

I assumed that I was the only one that needed CORS support. It works fine if your meteor project is deployed to "www.production.com" and your users log on to "www.production.com."

What I assume is unique in my case is that I am using multiple domain names. So mup/meteor gets deployed with "www.production.com", but my users logon to "mycustomprefix.production.com". Because meteor was deployed to "www.production.com", the file uploader code was built with a hard coded url for "www.production.com", which incurs a CORS issue. Thus, I had to put in an allowedOrigin of "*.production.com".

Then of course, as I stated initially, the file uploader code on the indicated line incorrectly evaluates true/false and discards my setting.

@dr-dimitru dr-dimitru added the bug label Nov 3, 2022
dr-dimitru added a commit that referenced this issue Nov 3, 2022
dr-dimitru added a commit that referenced this issue Nov 3, 2022
__Changes:__

- 👨‍💻 Improve `createIndex` helper
- 👨‍💻 Improve error output when FileSystem destination not writable; Related to #857, thanks to @Leekao
- 🐞 Fix custom `allowedOrigins` option for CORS; Closing #850, thanks to @djlogan2;

__Notes:__

- 👨‍🔬 Tested with latest release of `meteor@2.8.0`
@dr-dimitru dr-dimitru mentioned this issue Nov 3, 2022
dr-dimitru added a commit that referenced this issue Nov 3, 2022
v2.3.1

__Changes:__

- 👨‍💻 Improve `createIndex` helper
- 👨‍💻 Improve error output when FileSystem destination not writable; Related to #857, thanks to @Leekao
- 🐞 Fix custom `allowedOrigins` option for CORS; Closing #850, thanks to @djlogan2;

__Notes:__

- 👨‍🔬 Tested with latest release of `meteor@2.8.0`
@dr-dimitru
Copy link
Member

@djlogan2 thank you for reporting this and additional details you have provided. We have escalated this one to the bug level and its fix is available in v2.3.1

Feel free to close it in case if the issue is solved on your end.

@dr-dimitru
Copy link
Member

@djlogan2 have you had a chance to test v2.3.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants