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: warn on malformed URI query parameter #11489

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bryanhonof
Copy link
Member

@bryanhonof bryanhonof commented Sep 11, 2024

cc: @tomberek @roberth @Mic92

Motivation

Retry of #11349

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

if (pathEnd != std::string::npos && fragmentStart != std::string::npos) {
if (pathEnd != std::string::npos && fragmentStart != std::string::npos && url[pathEnd] == '?') {
Copy link
Member Author

Choose a reason for hiding this comment

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

It was possible to pass the fragment as a "query" with the following flakeRef.

.#bla

I don't think this was expected behavior, since the above doesn't have a query.

Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Could you add a test in functional that checks for the warning? Here is an example: d22a2fe
You can use grepQuiet for filtering.

@bryanhonof bryanhonof force-pushed the bryanhonof.warn-on-malformed-uri-query branch from f53b144 to a798569 Compare September 18, 2024 13:40
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Sep 18, 2024
@bryanhonof
Copy link
Member Author

bryanhonof commented Sep 18, 2024

Could you add a test in functional that checks for the warning? Here is an example: d22a2fe You can use grepQuiet for filtering.

@Mic92 I added some tests in tests/functional/flakes, and used the assertStderr function to test the output I'm expecting. Please let me know if this isn't what you were expecting.

@bryanhonof bryanhonof force-pushed the bryanhonof.warn-on-malformed-uri-query branch 2 times, most recently from f9cead0 to b04005a Compare September 18, 2024 14:37
@bryanhonof bryanhonof force-pushed the bryanhonof.warn-on-malformed-uri-query branch 3 times, most recently from 196c65e to 33b8aa6 Compare September 18, 2024 16:01
@bryanhonof
Copy link
Member Author

CI on macOS seems to been canceled whilst it was running.

@Mic92
Copy link
Member

Mic92 commented Sep 18, 2024

tests run locally on macOS:

Ok: 151
Expected Fail: 1
Fail: 2
Unexpected Pass: 0
Skipped: 29
Timeout: 0

@bryanhonof
Copy link
Member Author

I also ran a nix flake check on my aarch64-darwin machine, all seems to have passed just fine.

@bryanhonof bryanhonof force-pushed the bryanhonof.warn-on-malformed-uri-query branch from 33b8aa6 to 6c93bf1 Compare September 18, 2024 23:36
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

CI is green now.

infinisil added a commit to NixOS/SC-election-2024 that referenced this pull request Sep 20, 2024
Some of these are a bit after the specified time period, but I also believe some of these items are difficult to gauge how much "impact" they had.

Helped organize, host, and run, the Summer of Nix Lecture Series 2022

- https://www.youtube.com/playlist?list=PLt4-_lkyRrOMWyp5G-m_d1wtTcbBaOxZk
- NixOS/infra#213

Helped organize, and host infra for, the Summer of Nix Lecture Series 2023

- https://www.youtube.com/playlist?list=PLt4-_lkyRrOPcBuz_tjm6ZQb-6rJjU3cf
- NixOS/infra#240

Helped organize, host, and was responsible for livestreaming infra during, NixCon Paris 2022

- https://www.youtube.com/playlist?list=PLgknCdxP89ReD6gxl755B6G_CI65z4J2e

Maintenance of some nixpkgs packages

- NixOS/nixpkgs#340223 (contribution after 2024-05-01)
- NixOS/nixpkgs#290084
- NixOS/nixpkgs#170089

Organized, and assembled a team for the FOSDEM 2023 Nix/NixOS Devroom

- https://discourse.nixos.org/t/fosdem-2023-nix-and-nixos-devroom/23133

Organizer & sole maintainer of the Config Management Camp Nix track

- https://discourse.nixos.org/t/config-management-camp-2023-ghent/23455
- https://discourse.nixos.org/t/config-management-camp-2024-ghent/33852
- https://discourse.nixos.org/t/cfgmgmtcamp-2025-is-looking-for-nix-presentations/51658 (contribution after 2024-05-01)

Public speaking & spreading awareness of Nix/NixOS

- https://youtu.be/gUjvnZ9ZwMs?si=nDiZTCpQj53wwq8P
- https://www.youtube.com/watch?v=hNcYPH5Q_pA&t=862s

The occasional dabble into the Nix C++ code base

- NixOS/nix#11494 (contribution after 2024-05-01)
- NixOS/nix#11490 (contribution after 2024-05-01)
- NixOS/nix#11489 (contribution after 2024-05-01)
- NixOS/nix#11349 (contribution after 2024-05-01)
- NixOS/nix#11241 (contribution after 2024-05-01)
- NixOS/nix#9557
- NixOS/nix#8788
- NixOS/nix#8212
- NixOS/nix#5147

General evangelism

Pretty much every event I attend, I'm talking about Nix, showing off Nix/NixOS, and just trying to get people to see how awesome this tool is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants