-
Notifications
You must be signed in to change notification settings - Fork 4
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 configurable CORS policy #198
base: devel
Are you sure you want to change the base?
Conversation
cors({ | ||
origin: config.corsOrigin, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it makes sense to only limit origin in requests for sending transactions?
So that any read-only integrations won't break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, didn't think about possible read-only integrations.
@akolotov wdyt?
zp-relayer/configs/relayerConfig.ts
Outdated
@@ -14,6 +14,8 @@ const relayerAddress = new Web3().eth.accounts.privateKeyToAccount( | |||
process.env.RELAYER_ADDRESS_PRIVATE_KEY as string | |||
).address | |||
|
|||
const corsOrigin = (process.env.RELAYER_CORS_ORIGIN || '').split(' ').filter(url => url.length > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if it will parse and support regex patterns correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node's dotenv package parses everything as a string. It would be hard to tell if the given string is a regex to cast it to the RegExp
object. But, we can make a convention (like JS engine does), that anything enclosed in slashes (e.g. /$STRING$/
) will be considered as a regex. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind, let's just make sure that simple wildcards like *.zkbob.com
will work in one way or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for regexps. Any string enclosed in slashes will be casted to RegExp object and passed to cors middleware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, added a debug log with origins because there could be tricky cases. E.g. to get the analog of JS regexp /\w+/
you need to write the following string in env: /\\w+/
.
When using the constructor function, the normal string escape rules (preceding special characters with \ when included in a string) are necessary.
No description provided.