Skip to content
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

fix: handle InvalidLabelChar in URL validation to prevent panic #24080

Conversation

yazan-abdalrahman
Copy link
Contributor

@yazan-abdalrahman yazan-abdalrahman commented Jun 2, 2024

Fixes #23294
Fixes #23552

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2024

CLA assistant check
All committers have signed the CLA.

@yazan-abdalrahman yazan-abdalrahman changed the title handle InvalidLabelChar in URL validation to prevent panic fix(runtime): handle InvalidLabelChar in URL validation to prevent panic Jun 2, 2024
@yazan-abdalrahman yazan-abdalrahman changed the title fix(runtime): handle InvalidLabelChar in URL validation to prevent panic fix: handle InvalidLabelChar in URL validation to prevent panic Jun 3, 2024
ext/net/ops.rs Outdated Show resolved Hide resolved
ext/net/ops.rs Show resolved Hide resolved
@yazan-abdalrahman
Copy link
Contributor Author

@dsherret

After updating the branch and resolving the merge conflict issue, I encountered a cyclic dependence issue; how can we resolve it? I need to change  NetPermissions  place to avoid cycle dependence issue?

image
image

error: cyclic package dependency: package deno_permissions v0.15.0 (D:\yad\deno\runtime\permissions) depends on itself. Cycle:
package deno_permissions v0.15.0 (D:\yad\deno\runtime\permissions)
... which satisfies path dependency deno_permissions (locked to 0.15.0) of package deno_net v0.147.0 (D:\yad\deno\ext\net)
... which satisfies path dependency deno_net (locked to 0.147.0) of package deno_permissions v0.15.0 (D:\yad\deno\runtime\permissions)
... which satisfies path dependency deno_permissions (locked to 0.15.0) of package deno_web v0.186.0 (D:\yad\deno\ext\web)
... which satisfies path dependency deno_web (locked to 0.186.0) of package deno_crypto v0.169.0 (D:\yad\deno\ext\crypto)
... which satisfies path dependency deno_crypto (locked to 0.169.0) of package deno_runtime v0.163.0 (D:\yad\deno\runtime)
... which satisfies path dependency deno_runtime (locked to 0.163.0) of package deno v1.44.1 (D:\yad\deno\cli)

@yazan-abdalrahman
Copy link
Contributor Author

Hello @dsherret
, could you please review the latest changes?

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about removing new functionality.

mapping errors to URIErrors
'like the original behavior'.
@yazan-abdalrahman
Copy link
Contributor Author

yazan-abdalrahman commented Jun 25, 2024

See comment about removing new functionality.

@dsherret
done I deleted parsing URL as the origin behavior, mapped error to URIError as the origin instance of error and removed the file you mentioned as not relevant.

@yazan-abdalrahman
Copy link
Contributor Author

See comment about removing new functionality.

@dsherret
I removed the new feature of parsing URLs and the origin behavior in --allow-net=github.com:443. It gives permission for github.com at port 443 only, but when you do --allow-net=github.com, it will give permission at all ports, which is the original behavior, and this behavior works with IPv4, IPv6, and hostname.

I saw that via this reference:
https://docs.deno.com/runtime/manual/basics/permissions
https://deno.land/[email protected]?s=Deno.connect
https://deno.land/[email protected]?s=Deno.listen

If you need to remove any functions, it will defect the permission system.

@yazan-abdalrahman
Copy link
Contributor Author

@dsherret
Hello, can you re-run the pipeline

lucacasonato added a commit that referenced this pull request Jul 5, 2024
Also don't panic on invalid domain names and addresses.

Extracted with cleanups up from #24080

Co-authored-by: Yazan AbdAl-Rahman <[email protected]>
@bartlomieju
Copy link
Member

Closing in favor of #24397 which landed parts of this PR.

@bartlomieju bartlomieju closed this Jul 5, 2024
zebreus pushed a commit to zebreus/deno that referenced this pull request Jul 8, 2024
Also don't panic on invalid domain names and addresses.

Extracted with cleanups up from denoland#24080

Co-authored-by: Yazan AbdAl-Rahman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when passing wildcard to allow-net thread 'main' panicked at runtime/permissions/lib.rs:695:19
4 participants