Add lint to prevent (most) URLs #104 by KoxyG · Pull Request #106 · ethereum/eipw

KoxyG · 2024-09-04T22:55:45Z

Pls review and let me know if there is anything that needs changes @SamWilsn

SamWilsn · 2024-09-05T13:33:45Z

+         // Report the issue
+         let source = self.ctx.source_for_text(ast.sourcepos.start.line, text);
+         let offending_url = self.re.find(text).unwrap().as_str();
+         let suggestion_label = format!("Avoid using backticks in URLs: `{}`", offending_url);


Haven't investigated the code yet, but this text seems to indicate it does the opposite of what we need.

The lint is supposed to make sure authors don't put URLs in backticks, not that they don't put backticks in URLs.

SamWilsn · 2024-09-05T13:39:14Z

+ #[derive(Debug, Serialize, Deserialize, Clone)]
+ pub struct PreventUrlsNoBackticks<S>(pub S);


This should probably look something like:

Suggested change

#[derive(Debug, Serialize, Deserialize, Clone)]

pub struct PreventUrlsNoBackticks<S>(pub S);

#[derive(Debug, Serialize, Deserialize, Clone)]

pub struct PreventUrlsNoBackticks<S> {

allowed_domains: Vec<S>,

}

Then the default configuration in lib.rs would look like:

( "markdown-prevent-url", MarkdownPreventUrl { allowed_domains: vec![ r"(.+\.)?example.com", r"(.+\.)?invalid", // And so on... ] } ),

SamWilsn · 2024-09-05T13:42:12Z

+ header: value1
+ ---
+
+ Check this URL: `http://example.com/page?query=foo`


You should write tests based on the examples in #104. This URL is very close to one of the examples I gave that should not trigger the lint.

In other words, this URL should be allowed because its domain is example.com—one of the allowed example domains at the top of #104.

KoxyG · 2024-09-07T19:08:19Z

Hi @SamWilsn I just made changes. Kindly check and help review.

SamWilsn

Quick review. You'll need to resolve the compilation issues before we can go any further.

SamWilsn · 2024-09-30T19:53:37Z

+
+         // Regex to match URLs not from allowed domains and containing backtick
+
+        let re = Regex::new(r"https?://(?:example\.com|example\.net|example\.org|example|invalid|test)(?:/[^`\s]*)?").unwrap();


I haven't quite figured out what this regex is for yet, but you shouldn't hardcode the list of domains here. It could get out of sync with self.allowed_domains.

SamWilsn · 2024-09-30T19:57:54Z

+             // If the URL is in backticks, report it
+             if in_backticks {
+                let source = self.ctx.source_for_text(ast.sourcepos.start.line, text);
+                let message = format!("URLs are not allowed in backticks: `{}`", url);


Suggested change

let message = format!("URLs are not allowed in backticks: `{}`", url);

let message = "Valid URLs are not allowed in backticks";

I think we can use a simpler error message here.

SamWilsn · 2024-09-30T19:59:03Z

+                    title: Some(Annotation {
+                        annotation_type: self.ctx.annotation_type(),
+                        id: Some(self.slug),
+                        label: Some("not from allowed domain found"),


Suggested change

label: Some("not from allowed domain found"),

label: Some("non-reserved domain"),

How about that?

SamWilsn · 2024-09-30T20:00:39Z

+            // Report other disallowed URLs
+            let source = self.ctx.source_for_text(ast.sourcepos.start.line, text);
+            let suggestion_label = format!("This URL must be hyperlinked or from an allowed domain: `{}`", url);
+
+            self.ctx.report(Snippet {
+                opt: Default::default(),
+                title: Some(Annotation {
+                    annotation_type: self.ctx.annotation_type(),
+                    id: Some(self.slug),
+                    label: Some("Disallowed URL in plain text or code fence"),
+                }),
+                slices: vec![Slice {
+                    fold: false,
+                    line_start: ast.sourcepos.start.line,
+                    origin: self.ctx.origin(),
+                    source: &source,
+                    annotations: vec![],
+                }],
+                footer: vec![Annotation {
+                    annotation_type: AnnotationType::Help,
+                    id: None,
+                    label: Some(&suggestion_label),
+                }],
+            })?;


It's not immediately obvious why this section is separate from the above one. Could you explain?

SamWilsn · 2024-09-30T20:01:17Z

+ impl<'a, 'b, 'c> tree::Visitor for Visitor<'a, 'b, 'c> {
+     type Error = Error;
+
+     fn enter_link(&mut self, _ast: &Ast, _link: &str) -> Result<Next, Self::Error> {
+            Ok(Next::TraverseChildren)
+     }
+
+     fn enter_code(&mut self, ast: &Ast, code: &comrak::nodes::NodeCode) -> Result<Next, Self::Error> {
+         self.check(ast, &code.literal, false, false)
+     }
+
+     fn enter_text(&mut self, ast: &Ast, text: &str) -> Result<Next, Self::Error> {
+         self.check(ast, text, false, false)
+     }
+
+     fn enter_html_inline(&mut self, ast: &Ast, html: &str) -> Result<Next, Self::Error> {
+        // For inline HTML links like <https://link>
+        self.check(ast, html, true, false)
+    }
+
+    fn enter_code_block(&mut self, ast: &Ast, block: &comrak::nodes::NodeCodeBlock) -> Result<Next, Self::Error> {
+        let code_text = String::from_utf8_lossy(&block.literal);
+        self.check(ast, &code_text, false, false)
+    }
+
+
+ } 


You're missing a few cases here I think.

KoxyG added 2 commits September 4, 2024 23:47

worked on prevent url with backticks or that are not hyperlinked

7c0c704

Add lint to prevent (most) URLs ethereum#104

7cf8a4f

KoxyG mentioned this pull request Sep 5, 2024

Add lint to prevent (most) URLs #104

Open

SamWilsn requested changes Sep 5, 2024

View reviewed changes

KoxyG added 2 commits September 7, 2024 20:00

avoid url in backticks

71e2cc1

url in backticks

8a0af04

SamWilsn added 5 commits September 30, 2024 15:46

Update README.md

187d6e7

Update relative_links.rs

a93db36

Update README.md

a206b44

Update README.md

8a4d978

Update headings_space.rs

f78a08e

SamWilsn reviewed Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint to prevent (most) URLs #104#106

Add lint to prevent (most) URLs #104#106
KoxyG wants to merge 9 commits intoethereum:masterfrom
KoxyG:add_lint

KoxyG commented Sep 4, 2024

Uh oh!

SamWilsn Sep 5, 2024

Uh oh!

SamWilsn Sep 5, 2024

Uh oh!

SamWilsn Sep 5, 2024

Uh oh!

KoxyG commented Sep 7, 2024 •

edited

Loading

Uh oh!

SamWilsn left a comment

Uh oh!

SamWilsn Sep 30, 2024

Uh oh!

SamWilsn Sep 30, 2024

Uh oh!

SamWilsn Sep 30, 2024

Uh oh!

SamWilsn Sep 30, 2024

Uh oh!

SamWilsn Sep 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		#[derive(Debug, Serialize, Deserialize, Clone)]
		pub struct PreventUrlsNoBackticks<S>(pub S);


		// Regex to match URLs not from allowed domains and containing backtick

		let re = Regex::new(r"https?://(?:example\.com\|example\.net\|example\.org\|example\|invalid\|test)(?:/[^`\s]*)?").unwrap();

	let message = format!("URLs are not allowed in backticks: `{}`", url);
	let message = "Valid URLs are not allowed in backticks";

	label: Some("not from allowed domain found"),
	label: Some("non-reserved domain"),

Conversation

KoxyG commented Sep 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KoxyG commented Sep 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamWilsn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KoxyG commented Sep 7, 2024 •

edited

Loading