-
Notifications
You must be signed in to change notification settings - Fork 194
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
Fixes some issues reported by linter. #303
Conversation
@@ -242,21 +243,21 @@ export class CloudflareProvider implements Provider { | |||
// header according to https://xhr.spec.whatwg.org/#dom-xmlhttprequest-setrequestheader | |||
// So we need to extract the token from the Referer header and send it in the query | |||
// param __cf_chl_f_tk instead. (Note that this token is not a Privacy Pass token. | |||
let token: string | null = null; | |||
let atoken: string | null = null; |
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.
What does this mean? Is it a lint error? I cannot find it from npm run lint
?
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 think token
(without s) already means a token.
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.
Linter reports security/detect-possible-timing-attacks
when detects a variable named token
or password
.
@@ -335,14 +340,14 @@ export class CloudflareProvider implements Provider { | |||
|
|||
// Get one token. | |||
const tokens = this.getStoredTokens(); | |||
const token = tokens.shift(); | |||
const oneToken = tokens.shift(); |
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 think token
(without s) already means one token.
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.
the issue is that the linter looks for variables named token
or password
and warns about it. The fix proposed is to rename the variable.
if (details.requestBody.formData[key].length == 1) { | ||
const [value] = details.requestBody.formData[key]; | ||
flattenFormData[key] = value; | ||
if (details.requestBody.formData[key as string].length == 1) { |
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.
This seems to be a vulnerability rather than a lint error. https://github.com/nodesecurity/eslint-plugin-security/blob/master/docs/the-dangers-of-square-bracket-notation.md
This patch makes the linter pass rather fixing the vulnerability. https://github.com/nodesecurity/eslint-plugin-security/blob/master/docs/the-dangers-of-square-bracket-notation.md#how-do-i-fix-it
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 think defining a function checking for the key of constructor
, __proto__
, and prototype
should be fine.
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.
According to the reference:
How do I fix it?
The most direct fix here is going to be to avoid the use of user input in property name fields. This isn't reasonable in all circumstances, however, and there should be a way to safely use core language features.
so, we need to refactor code to avoid property name fields. Open to suggestions for this change.
fddbed1
to
2fff801
Compare
027fd99
to
1ee8d5f
Compare
1ee8d5f
to
a6d6c6b
Compare
a6d6c6b
to
961b561
Compare
No description provided.