-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
feat(linter): implement jsx-no-script-url
#6995
base: main
Are you sure you want to change the base?
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
CodSpeed Performance ReportMerging #6995 will not alter performanceComparing Summary
|
|
||
fn jsx_no_script_url_diagnostic(span: Span) -> OxcDiagnostic { | ||
// See <https://oxc.rs/docs/contribute/linter/adding-rules.html#diagnostics> for details | ||
OxcDiagnostic::warn("A future version of React will block javascript: URLs as a security precaution. Use event handlers instead if you can. If you need to generate unsafe HTML, try using dangerouslySetInnerHTML instead.") |
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.
Move all sentences but the first to a help message
/// | ||
/// ### Why is this bad? | ||
/// | ||
/// In React 16.9 any URLs starting with javascript: scheme log a warning. React considers the pattern as a dangerous attack surface, see details. In a future major release, React will throw an error if it encounters a javascript: URL. |
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 details?
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.
Copy paste issue, will change
return; | ||
}; | ||
if prop_value.as_string_literal().is_some_and(|val| { | ||
let re = Regex::new(IS_JAVA_SCRIPT_PROTOCOL).unwrap(); |
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.
you should use lazy_static
to wrap it to make sure it is only initialized once. ref
oxc/crates/oxc_linter/src/rules/eslint/no_nonoctal_decimal_escape.rs
Lines 87 to 90 in a786dc2
lazy_static! { | |
static ref NONOCTAL_REGEX: Regex = | |
Regex::new(r"(?:[^\\]|(?P<previousEscape>\\.))*?(?P<decimalEscape>\\[89])").unwrap(); | |
} |
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.
here is the reason, https://github.com/rust-lang/regex?tab=readme-ov-file#usage-avoid-compiling-the-same-regex-in-a-loop, I recommended using lazy_static
since it is widely used in linter
crate
3e32476
to
a0ddd86
Compare
a0ddd86
to
6775cf7
Compare
), | ||
), | ||
( | ||
r#"<Foo other="javascript:"></Foo>"#, |
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.
FYI, I added an extra test to pass
, as it checks for the link attributes not to have the javascript:
value.(It seems that the original implementation of the rule does that, it just doesn't have a test case for it). I can remove it though if deemed as not necessary.
#1022