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.

Sign up for GitHub

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

Jump to bottom

fix: warn on malformed URI query parameter #11489

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

fix: warn on malformed URI query parameter #11489

wants to merge 2 commits into from

Conversation

bryanhonof
Copy link
Member

@bryanhonof bryanhonof commented Sep 11, 2024 •
edited
Loading

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.

bryanhonof
bryanhonof commented Sep 11, 2024
View reviewed changes
src/libflake/flake/flakeref.cc
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.

Mic92
Mic92 reviewed Sep 18, 2024
View reviewed changes
Copy link
Member

@Mic92 Mic92 left a comment •
edited
Loading

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 •
edited
Loading

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
Mic92
Mic92 approved these changes Sep 19, 2024
View reviewed changes
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 approved these changes

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

Assignees
No one assigned
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bryanhonof @Mic92