fix(runtime): Improve Error Handling and-Dot Handling in-fqdn! Macro #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

…-Handling-in-fqdn!-Macro

…g-and-Dot-Handling-in-fqdn!-Macro

…tional; enable net access for host on specified port - Enhanced parsing logic for IPv6 and IPv4 addresses, as well as hostnames and URLs - Made port extraction optional by supporting the ":port" syntax in hostnames - Ensured consistent modification of `IpAddr` struct without cloning - Added error handling for invalid IPv6 format and missing port specifications - Enabled network access for the specified host on the given port - Refactored helper functions for better readability and maintainability

…-Handling-in-fqdn!-Macro # Conflicts: # Cargo.lock # ext/net/Cargo.toml

…-Handling-in-fqdn!-Macro

dsherret · 2024-05-23T13:53:23Z

cli/args/flags.rs

+  #[test]
+  fn wildcard_flags() {
+    #[rustfmt::skip]
+        let r = flags_from_vec(svec![


Suggested change

let r = flags_from_vec(svec![

let r = flags_from_vec(svec![

dsherret · 2024-05-23T13:54:07Z

ext/net/Cargo.toml

 pin-project.workspace = true
 rustls-tokio-stream.workspace = true
 serde.workspace = true
 socket2.workspace = true
 tokio.workspace = true
 trust-dns-proto = "0.22"
 trust-dns-resolver = { version = "0.22", features = ["tokio-runtime", "serde-config"] }
+url = "2.4.1"


Can you remove this and instead import Url from deno_core::url::Url?

dsherret · 2024-05-23T14:23:25Z

ext/net/lib.rs

 use std::sync::Arc;

 pub const UNSTABLE_FEATURE_NAME: &str = "net";

+#[derive(Clone, Eq, PartialEq, Hash, Debug)]
+pub struct NetPermissionHost {
+  pub host: String,


It's not ideal that we're going from a string to a known struct, to a string and then on each permission check doing the same thing. This code below here is also duplicated in permissions/lib.rs. Is there any way that once we parse something as a FQDN that we leave it as one? It seems like we might need to use an enum here to store multiple different kinds of values (ex. FQDN or IpAddr)

Hi @dsherret
dd15f60
done

dsherret · 2024-05-23T14:24:56Z

runtime/permissions/lib.rs

+  ///
+  /// # Errors
+  ///
+  /// Returns an `AnyError` if the input string cannot be parsed into a valid host and optional port.


This documentation comment is a lot to maintain.

…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

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

yazan-nidal added 2 commits May 9, 2024 09:44

Fix lint issues

92a9509

fmt

d7a541d

yazan-nidal requested a review from bartlomieju May 9, 2024 08:15

dsherret reviewed May 9, 2024

View reviewed changes

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

yazan-nidal and others added 3 commits May 12, 2024 16:07

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

c74d45f

…-Handling-in-fqdn!-Macro

Merge branch 'denoland:main' into fix-(runtime)-Improve-Error-Handlin…

1bbc4f6

…g-and-Dot-Handling-in-fqdn!-Macro

yazan-nidal requested a review from dsherret May 23, 2024 06:20

yazan-nidal and others added 2 commits May 23, 2024 09:43

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

6f8664b

…-Handling-in-fqdn!-Macro # Conflicts: # Cargo.lock # ext/net/Cargo.toml

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

40fe3bb

…-Handling-in-fqdn!-Macro

dsherret reviewed May 23, 2024

View reviewed changes

dsherret mentioned this pull request May 23, 2024

Deno panic with wildcard in --allow-net value #23963

Closed

mmastrac removed request for mmastrac and 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

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

3008de6

…-Handling-in-fqdn!-Macro

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yazan-nidal commented Apr 28, 2024 •

edited

CLAassistant commented Apr 28, 2024 •

edited

mmastrac commented Apr 28, 2024

yazan-nidal commented Apr 28, 2024 •

edited

yazan-nidal commented Apr 29, 2024 •

edited

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

dsherret May 23, 2024

dsherret May 23, 2024

yazan-nidal May 27, 2024

dsherret May 23, 2024

yazan-nidal May 27, 2024

dsherret May 23, 2024

yazan-nidal May 27, 2024

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

bartlomieju May 27, 2024

yazan-nidal May 28, 2024 •

edited

bartlomieju May 27, 2024

yazan-nidal May 28, 2024

bartlomieju May 27, 2024

yazan-nidal May 28, 2024

yazan-nidal commented May 28, 2024

		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 #23590

Are you sure you want to change the base?

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

Conversation

yazan-nidal commented Apr 28, 2024 • edited

CLAassistant commented Apr 28, 2024 • edited

mmastrac commented Apr 28, 2024

yazan-nidal commented Apr 28, 2024 • edited

yazan-nidal commented Apr 29, 2024 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yazan-nidal May 28, 2024 • edited

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-nidal commented Apr 28, 2024 •

edited

CLAassistant commented Apr 28, 2024 •

edited

yazan-nidal commented Apr 28, 2024 •

edited

yazan-nidal commented Apr 29, 2024 •

edited

yazan-nidal May 28, 2024 •

edited

yazan-nidal May 28, 2024 •

edited