fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro (closed) #23590

yazan-nidal · 2024-04-28T12:53:37Z

Implement improvements for error handling and dot handling in fqdn! macro

This commit addresses issue (#23294 , #23552) by enhancing the error handling and dot handling logic within the fqdn! macro. Specifically, it:

Handles parsing errors gracefully using pattern matching instead of unwrap().
Adjusts substring parsing to correctly handle domain names ending with a dot.

This change improves the reliability and robustness of the fqdn! macro, ensuring more accurate parsing of fully qualified domain names.

Fixes: #23294 , #23552

…acro This commit addresses issue denoland#23294 by enhancing the error handling and dot handling logic within the fqdn! macro. Specifically, it: - Handles parsing errors gracefully using pattern matching instead of unwrap(). - Adjusts substring parsing to correctly handle domain names ending with a dot. This change improves the reliability and robustness of the fqdn! macro, ensuring more accurate parsing of fully qualified domain names. Fixes: denoland#23294

CLAassistant · 2024-04-28T12:53:43Z

All committers have signed the CLA.

mmastrac · 2024-04-28T14:33:50Z

Thanks for the fix. Could you add some tests around the error cases this fixes?

Does this help w/ #23552 ?

yazan-nidal · 2024-04-28T14:46:27Z

Thanks for the fix. Could you add some tests around the error cases this fixes?

Does this help w/ #23552 ?

hi @mmastrac

Thank you for reviewing the fix! Since this is my first commit in Deno, I'm happy to add tests if necessary. Could you please guide me on how to locate existing tests in the codebase?

I tested the fix manually, but I'm keen to learn how tests are typically structured and where I can find examples within our project. This will help me understand the testing conventions used here and ensure that I contribute effectively to the test suite.

If you could point me to where I can find old tests or provide any resources on writing tests in Deno, I'd greatly appreciate it. Once I have a better understanding, I'll be more than happy to add the necessary tests for this fix.

Regarding issue #23552, I'm eager to ensure that our tests cover relevant error cases and edge scenarios.

Looking forward to your guidance!

…-Handling-in-fqdn!-Macro

yazan-nidal · 2024-04-29T13:03:02Z

@mmastrac
@bartlomieju
can u please review this pr?
I am not able to add reviewer.

0f-0b

This change would introduce a security hole.

$ cat a.js
const message = "AAABAAABAAAAAAAAB2V4YW1wbGUDbmV0AAAcAAE=";

// A program can trick the user into granting access to a single IPv6 address
console.log(
  await (await fetch(`https://[2606:4700:4700::1111]/dns-query?dns=${message}`))
    .arrayBuffer(),
);

// Now it can connect to the entire IPv6 network
console.log(
  await (await fetch(`https://[2606:4700:4700::1001]/dns-query?dns=${message}`))
    .arrayBuffer(),
);
$ target/debug/deno run a.js
✅ Granted net access to "[2606:4700:4700::1111]".
ArrayBuffer {
  [Uint8Contents]: <00 00 81 80 00 01 00 01 00 00 00 00 07 65 78 61 6d 70 6c 65 03 6e 65 74 00 00 1c 00 01 c0 0c 00 1c 00 01 00 00 0e 10 00 10 26 06 28 00 02 1f cb 07 68 20 80 da af 6b 8b 2c>,
  byteLength: 57
}
ArrayBuffer {
  [Uint8Contents]: <00 00 81 80 00 01 00 01 00 00 00 00 07 65 78 61 6d 70 6c 65 03 6e 65 74 00 00 1c 00 01 c0 0c 00 1c 00 01 00 00 0e 10 00 10 26 06 28 00 02 1f cb 07 68 20 80 da af 6b 8b 2c>,
  byteLength: 57
}

Note that the second fetch call proceeds without asking for permission.

yazan-nidal · 2024-04-29T13:59:12Z

Note that the second fetch call proceeds without asking for permission.

hi @0f-0b ,

It is for the same site, it will not request it the next time after approval the first time, and if it refuses, it will be rejected any other time, but it will ask you again if you submit a new request for another domain, and this is the original behavior, I just fixed it to make it not panic when using the wrong domain for fqdn! Macro, this did not affect the flow or behavior and was just to prevent panic

runtime/permissions/lib.rs

mmastrac

Requires some more work:

We can't return FQDN::default from parsing because that results in a broken permission descriptor
This will need some tests added to flags.rs, and spec tests added in tests/specs/run to ensure that it works as expected.
We'll also need to implement wildcard permission tests which we don't support -- ideally we'd return a better result from flags.rs when encountering these.

…n! macro This commit addresses issue (denoland#23294 , denoland#23552) by enhancing the error handling and dot handling logic within the fqdn! macro. Specifically, it: Handles parsing errors gracefully using pattern matching instead of unwrap(). Adjusts substring parsing to correctly handle domain names ending with a dot. This change improves the reliability and robustness of the fqdn! macro, ensuring more accurate parsing of fully qualified domain names. Fixes: denoland#23294 , denoland#23552

…-Handling-in-fqdn!-Macro

yazan-nidal · 2024-05-06T14:20:26Z

@mmastrac
@bartlomieju
@0f-0b

Please review the latest changes.

…-Handling-in-fqdn!-Macro

Fixes: denoland#23294 , denoland#23552

…-Handling-in-fqdn!-Macro

bartlomieju

@yazan-nidal there's a few linter errors - most of them are about use of eprintln! (which is now disallowed since 47f7bed) and a few because of using double reference. You can run ./tools/lint.js --rs locally to see these errors.

runtime/permissions/lib.rs

…n-fqdn!-Macro' of https://github.com/yazan-nidal/deno into fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro

…-Handling-in-fqdn!-Macro

bartlomieju

Very nice work, this is a really though nut to crack for the first PR, but you're very close.

A few more comments that needs to be fixed before we can land this one.

bartlomieju · 2024-05-27T21:25:11Z

ext/net/lib.rs

+}
+
+impl NetPermissionHost {
+  pub fn from_str(host: &str, mut port: Option<u16>) -> Result<Self, AnyError> {


I think clippy will complain about the name of this method. I suggest to rename it to from_host_and_maybe_port. Also port should be a mutable variable, do a copy if you need to change it

bartlomieju · 2024-05-27T21:25:51Z

ext/net/lib.rs

+        .to_string(),
+      "Invalid format: [ipv6]:port"
+    );
+  }


Very nice tests 👍

bartlomieju · 2024-05-27T21:26:03Z

ext/net/net.rs

Can you please rename this file to host.rs?

bartlomieju · 2024-05-27T21:26:20Z

ext/net/net.rs

+}
+
+impl Host {
+  pub fn from_str(host: &str, origin_host: &str) -> Result<Self, AnyError> {


Ditto, clippy will complain. Suggest to rename it to from_host_and_origin_host

bartlomieju · 2024-05-27T21:26:38Z

ext/net/net.rs

+    return Ok(Host::FQDN(host));
+  }
+
+  pub fn to_string(&self) -> String {


You might have to implement Display trait instead, but I'm not sure about clippy

bartlomieju · 2024-05-27T21:27:24Z

ext/net/net.rs

+  Ok((host, port))
+}
+
+pub fn extract_host(s: &str) -> String {


Is this meant only for URLs? If so maybe we should try to parse as url::Url and use appropriate methods from that crate? WDYT?

I extract the host and port from the url if it was url to request permission or grant permission to this host on this port so we don't need to parse the url itself
The url may contain another part such as the resource path

Likes
https://github.com:443/denoland/deno

I'm extracting "github.com:443" so we need permission to host:github.com on port:443

So I don't give any importance to the URL resource,

We also have another method to parse the url itself
"from_url"
But this needs a valid url

So I extracted the host from the url to validate the host

Such as https://git@hub.com/denoland/deno
git@hub.com -> Invalid

........

I tried parsing the url using url::Url and it gave no error for invalid url so I need to extract the host this way and then check the host is valid

bartlomieju · 2024-05-27T21:28:16Z

ext/net/ops.rs

+fn process_ip_addr(addr: &mut IpAddr) -> Result<(), AnyError> {
+  let extracted_host = extract_host(&addr.hostname);
+  let tmp_host = &extracted_host.clone();
+  let (host_str, port) = split_host_port(&extracted_host)?;
+  addr.hostname = Host::from_str(&host_str, &tmp_host)?.to_string();
+  if let Some(port) = port {
+    addr.port = port;
+  }
+  Ok(())
+}


What is the purpose of this function? A docstring here would be very useful, also if it's important there should be a unit test for it. I'm also not keen on having a mutable reference here if it's a validation function that doesn't return a value.

It is an handle for host before sent it to
01_net.js

When I try to do

Deno.connect({ hostname: "https://192.0.2.1/", port: 80 });
Deno.listen({ hostname: "https://golang.org/", port: 80, transport: "tcp" });

It will send the hostname and port as args
to
async function connect(args)
function listen(args)
In ext/net/01_net.js

So it will try to connect to invalid addresses, so we need to deal with the host and extract the valid port and host before connecting

bartlomieju · 2024-05-27T21:30:17Z

tests/specs/run/net_descriptor/err_iii.out

+error: Uncaught (in promise) ConnectionRefused: No connection could be made because the target machine actively refused it. (os error 10061)
+    at async Object.connect (ext:deno_net/01_net.js:587:55)


I'm a bit confused here. The provided permission and host/port pair matches in the ipv6.js file, then why is it erroring?

it's refused connection from the host of ipv6 not permission error i don't find any valid ipv6 can make connect with it at some port

bartlomieju · 2024-05-27T21:31:06Z

runtime/permissions/lib.rs

+  /// # Errors
+  ///
+  /// Returns an `AnyError` if the input string cannot be parsed.
+  pub fn from_str(s: &str) -> Result<Self, AnyError> {


Again, I think clippy will complain about this one. Is this one meant to parse the CLI args? If so I suggest to rename it to from_cli_arg()

it's for extract the net descriptor from str

when do --allow-net=https://github.com:443/

it will extract the the NetDescriptor(hostname:github.com ,port:443)

…-Handling-in-fqdn!-Macro

yazan-nidal · 2024-05-28T08:42:36Z

Very nice work, this is a really though nut to crack for the first PR, but you're very close.

A few more comments that needs to be fixed before we can land this one.

@bartlomieju
done
b4b84ab

…-Handling-in-fqdn!-Macro

…n-fqdn!-Macro' of https://github.com/yazan-nidal/deno into fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro

…-Handling-in-fqdn!-Macro

…n-fqdn!-Macro' of https://github.com/yazan-nidal/deno into fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro

…-Handling-in-fqdn!-Macro

…ling-and-Dot-Handling-in-fqdn!-Macro' into fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro

…and#23590

Resolves denoland#23590

yazan-abdalrahman · 2024-06-03T07:18:20Z

Hello all, it's the same pull request, but I'm pushing it with a Exalt account.
#24080

yazan-nidal closed this Apr 28, 2024

yazan-nidal reopened this Apr 28, 2024

yazan-nidal changed the title ~~Implement improvements for error handling and dot handling in fqdn! m…~~ Implement improvements for error handling and dot handling in fqdn! macro Apr 28, 2024

yazan-nidal changed the title ~~Implement improvements for error handling and dot handling in fqdn! macro~~ fix (runtime) Improve Error Handling and-Dot Handling in-fqdn! Macro Apr 28, 2024

mmastrac changed the title ~~fix (runtime) Improve Error Handling and-Dot Handling in-fqdn! Macro~~ fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro Apr 28, 2024

yazan-nidal and others added 3 commits April 29, 2024 12:42

add tests for modified fqdn

01b8f01

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

cc8a798

…-Handling-in-fqdn!-Macro

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

3aa5ca0

…-Handling-in-fqdn!-Macro

yazan-nidal mentioned this pull request Apr 29, 2024

Panic when passing wildcard to allow-net #23552

Open

0f-0b suggested changes Apr 29, 2024

View reviewed changes

mmastrac reviewed Apr 29, 2024

View reviewed changes

runtime/permissions/lib.rs Outdated Show resolved Hide resolved

mmastrac suggested changes Apr 29, 2024

View reviewed changes

yazan-nidal and others added 2 commits May 6, 2024 17:07

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

2fbaf97

…-Handling-in-fqdn!-Macro

yazan-nidal and others added 3 commits May 7, 2024 09:22

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

f8bfa41

…-Handling-in-fqdn!-Macro

add failed parse domain tests

589d407

Fixes: denoland#23294 , denoland#23552

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

9e373ce

…-Handling-in-fqdn!-Macro

yazan-nidal requested review from mmastrac and 0f-0b May 7, 2024 13:59

yazan-nidal and others added 3 commits May 8, 2024 10:02

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

1bbf516

…-Handling-in-fqdn!-Macro

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

114cc7a

…-Handling-in-fqdn!-Macro

fmt

694aa25

bartlomieju reviewed May 9, 2024

View reviewed changes

runtime/permissions/lib.rs Outdated Show resolved Hide resolved

mmastrac removed the request for review from 0f-0b May 23, 2024 16:20

yazan-nidal added 2 commits May 27, 2024 14:46

Merge branch 'fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-i…

816b410

…n-fqdn!-Macro' of https://github.com/yazan-nidal/deno into fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro

enhancing

dd15f60

yazan-nidal requested a review from dsherret May 27, 2024 13:13

yazan-nidal and others added 3 commits May 27, 2024 16:29

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

2179fe8

…-Handling-in-fqdn!-Macro

update Cargo.lock

49c4053

fmt

e7a41b0

bartlomieju reviewed May 27, 2024

View reviewed changes

yazan-nidal and others added 2 commits May 28, 2024 11:40

fmt#

b4b84ab

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

5c63b47

…-Handling-in-fqdn!-Macro

yazan-nidal requested a review from bartlomieju May 28, 2024 09:50

yazan-nidal and others added 10 commits May 28, 2024 12:56

fmt#

d704574

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

3008de6

…-Handling-in-fqdn!-Macro

Merge branch 'fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-i…

233111f

…n-fqdn!-Macro' of https://github.com/yazan-nidal/deno into fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro

fmt

8b8e240

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

58efbb5

…-Handling-in-fqdn!-Macro

fmt#

932efa7

Merge branch 'fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-i…

68c6104

…n-fqdn!-Macro' of https://github.com/yazan-nidal/deno into fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro

Merge branch 'main' into fix-(runtime)-Improve-Error-Handling-and-Dot…

280035e

…-Handling-in-fqdn!-Macro

lint

05c6689

Merge remote-tracking branch 'origin/fix-(runtime)-Improve-Error-Hand…

390b79c

…ling-and-Dot-Handling-in-fqdn!-Macro' into fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro

yazan-abdalrahman pushed a commit to yazan-abdalrahman/deno that referenced this pull request Jun 2, 2024

fix: handle InvalidLabelChar in URL validation to prevent panic denol…

2897729

…and#23590

yazan-nidal closed this Jun 2, 2024

yazan-nidal changed the title ~~fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro~~ fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro (closed) Jun 2, 2024

yazan-nidal deleted the fix-(runtime)-Improve-Error-Handling-and-Dot-Handling-in-fqdn!-Macro branch June 2, 2024 11:07

yazan-abdalrahman pushed a commit to yazan-abdalrahman/deno that referenced this pull request Jun 2, 2024

handle InvalidLabelChar in URL validation to prevent panic

76304fd

Resolves denoland#23590

yazan-abdalrahman pushed a commit to yazan-abdalrahman/deno that referenced this pull request Jun 2, 2024

handle InvalidLabelChar in URL validation to prevent panic

947b37e

Resolves denoland#23590

yazan-abdalrahman mentioned this pull request Jun 3, 2024

thread 'main' panicked at runtime/permissions/lib.rs:695:19 #23294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro (closed) #23590

fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro (closed) #23590

yazan-nidal commented Apr 28, 2024 •

edited

Loading

CLAassistant commented Apr 28, 2024 •

edited

Loading

mmastrac commented Apr 28, 2024

yazan-nidal commented Apr 28, 2024 •

edited

Loading

yazan-nidal commented Apr 29, 2024 •

edited

Loading

0f-0b left a comment

yazan-nidal commented Apr 29, 2024

mmastrac left a comment

yazan-nidal commented May 6, 2024

bartlomieju left a comment

bartlomieju left a comment

bartlomieju May 27, 2024

yazan-nidal May 28, 2024

bartlomieju May 27, 2024

bartlomieju May 27, 2024

yazan-nidal May 28, 2024

bartlomieju May 27, 2024

bartlomieju May 27, 2024

bartlomieju May 27, 2024

yazan-nidal May 28, 2024 •

edited

Loading

bartlomieju May 27, 2024

yazan-nidal May 28, 2024 •

edited

Loading

bartlomieju May 27, 2024

yazan-nidal May 28, 2024

bartlomieju May 27, 2024

yazan-nidal May 28, 2024

yazan-nidal commented May 28, 2024

yazan-abdalrahman commented Jun 3, 2024 •

edited

Loading

		error: Uncaught (in promise) ConnectionRefused: No connection could be made because the target machine actively refused it. (os error 10061)
		at async Object.connect (ext:deno_net/01_net.js:587:55)

fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro (closed) #23590

fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro (closed) #23590

Conversation

yazan-nidal commented Apr 28, 2024 • edited Loading

CLAassistant commented Apr 28, 2024 • edited Loading

mmastrac commented Apr 28, 2024

yazan-nidal commented Apr 28, 2024 • edited Loading

yazan-nidal commented Apr 29, 2024 • edited Loading

0f-0b left a comment

Choose a reason for hiding this comment

yazan-nidal commented Apr 29, 2024

mmastrac left a comment

Choose a reason for hiding this comment

yazan-nidal commented May 6, 2024

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yazan-nidal May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yazan-nidal May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yazan-nidal commented May 28, 2024

yazan-abdalrahman commented Jun 3, 2024 • edited Loading

yazan-nidal commented Apr 28, 2024 •

edited

Loading

CLAassistant commented Apr 28, 2024 •

edited

Loading

yazan-nidal commented Apr 28, 2024 •

edited

Loading

yazan-nidal commented Apr 29, 2024 •

edited

Loading

yazan-nidal May 28, 2024 •

edited

Loading

yazan-nidal May 28, 2024 •

edited

Loading

yazan-abdalrahman commented Jun 3, 2024 •

edited

Loading