Skip to content

Comments

Warn on malformed URI query parameter#11349

Merged
roberth merged 2 commits intoNixOS:masterfrom
bryanhonof:bryanhonof.check-query-for-equals
Aug 28, 2024
Merged

Warn on malformed URI query parameter#11349
roberth merged 2 commits intoNixOS:masterfrom
bryanhonof:bryanhonof.check-query-for-equals

Conversation

@bryanhonof
Copy link
Member

@bryanhonof bryanhonof commented Aug 21, 2024

Motivation

#10370 explains how github:nixos/nixpkgs?refnixpkgs-unstable is currently just accepted as is.
It never really checks if the query part of the URI is a tuple of 2 strings, separated by an equal sign.

Context

Fixes #10370

Priorities and Process

Add 👍 to pull requests you find important.

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

@bryanhonof bryanhonof requested a review from edolstra as a code owner August 21, 2024 14:59
@bryanhonof bryanhonof force-pushed the bryanhonof.check-query-for-equals branch from 2caba30 to e7eb55b Compare August 21, 2024 15:08
@bryanhonof bryanhonof changed the title Error on malformed URI query parameter Warn on malformed URI query parameter Aug 21, 2024
@bryanhonof bryanhonof force-pushed the bryanhonof.check-query-for-equals branch from e7eb55b to 9decb14 Compare August 21, 2024 15:10
@bryanhonof
Copy link
Member Author

bryanhonof commented Aug 21, 2024

Outputs looks like this:

$ ./outputs/out/bin/nix --extra-experimental-features 'nix-command flakes' flake metadata 'github:nixos/nixpkgs?refnixpkgs-unstable&bla'
warning: invalid URI query 'refnixpkgs-unstable', did you forget an equals sign `=`?
warning: invalid URI query 'bla', did you forget an equals sign `=`?

@bryanhonof bryanhonof force-pushed the bryanhonof.check-query-for-equals branch from 9decb14 to c9f4567 Compare August 23, 2024 20:04
@tomberek
Copy link
Contributor

tomberek commented Aug 26, 2024

This will restrict all URIs from using non-key/value query parameters. So even http endpoints will not accept them. If we warn or error, it would be specific to the kind of URL it is, flake or store or fetch url, etc.

@bryanhonof
Copy link
Member Author

bryanhonof commented Aug 26, 2024 •
edited
Loading

@tomberek So, you'd rather do the check on the individual store level?
Because, I don't think these kinds of queries ever make it outside of parsing.

Since this checks if the string contains a =, and if it doesn't, just skips it.

nix/src/libutil/url.cc

Lines 82 to 85 in 96a2dda

if (e != std::string::npos)
result.emplace(
s.substr(0, e),
percentDecode(std::string_view(s).substr(e + 1)));

So, if there's already existing behavior depending on queries that don't have an = in them, I'd like to better understand how it works.

@tomberek
Copy link
Contributor

tomberek commented Aug 27, 2024

Fair point. In that case this is strictly improving the warnings. I'm not sure there are too many usages of the non-"=" query string.

roberth
roberth reviewed Aug 28, 2024
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

LGTM

@bryanhonof @roberth
Update src/libutil/url.cc
9b1cefe 
Co-authored-by: Robert Hensing <[email protected]>
@nixos-discourse
Copy link

nixos-discourse commented Aug 28, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-08-26-nix-team-meeting-minutes-172/51300/1

@roberth roberth merged commit b89eca9 into NixOS:master Aug 28, 2024
@nixos-discourse
Copy link

nixos-discourse commented Aug 28, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-08-28-nix-team-meeting-minutes-173/51302/1

@bryanhonof bryanhonof deleted the bryanhonof.check-query-for-equals branch August 28, 2024 21:25
@Mic92
Copy link
Member

Mic92 commented Sep 5, 2024

I am getting warnings now for simple cases like this:

nix-shell-env % nix build .#uefi-firmware
warning: dubious URI query 'uefi-firmware' is missing equal sign '='
[1/12/16 built, 31 copied (1333.3 MiB), 286.1 MiB DL] building edk2-src: removing `.git'...

basically any flake attribute.

@Mic92
Copy link
Member

Mic92 commented Sep 5, 2024

Revert in #11439

@tomberek
Copy link
Contributor

tomberek commented Sep 5, 2024

Might need an initial check to see if the query string is empty.

@bryanhonof bryanhonof mentioned this pull request Sep 11, 2024
Merged
infinisil added a commit to NixOS/SC-election-2024 that referenced this pull request Sep 20, 2024
@infinisil
Approve exception request for @bryanhonof
f12c8f1 
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

Reviewers

@tomberek tomberek tomberek approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

+1 more reviewer

@roberth roberth roberth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Nix team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

warn about unknown flake url paramaters

5 participants

@bryanhonof @tomberek @nixos-discourse @Mic92 @roberth