Accept a list item via ?item= query parameter when the URI path is empty - #2953
Open
DL6ER wants to merge 6 commits into
Open
Accept a list item via ?item= query parameter when the URI path is empty#2953DL6ER wants to merge 6 commits into
?item= query parameter when the URI path is empty#2953DL6ER wants to merge 6 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
DL6ER
force-pushed
the
fix/list-item-query-fallback
branch
from
July 12, 2026 13:20
a270067 to
2d57d57
Compare
5 tasks
…empty A domain, regex, or group whose value is literally `.` or `..` cannot be edited or deleted from the web UI. Per the WHATWG URL standard a path segment equal to `.`/`%2e` (single-dot) or `..`/`%2e%2e` (double-dot) is removed while the browser resolves the URL, so such an item is stripped from every request before it is sent. Percent-encoding the dot does not help, as `%2e` is normalized identically to `.`. The item cannot travel as a path component in these cases, so the web identifies it with an `?item=...` query parameter instead, which is not subject to path normalization. Honor that parameter in `api_list()` whenever the routed path leaves `api->item` empty. `POST` intentionally carries the item in the payload and is left untouched; the change covers `GET`/`PUT`/`DELETE` for every list type. The value is decoded into a buffer sized from the raw query string. As the URL-decoded result is never longer than its encoding, this can never truncate, even for long regular expressions or adlist URLs. The dispatch now has a single exit so the buffer is freed exactly once. See pi-hole/web#3308 Signed-off-by: DL6ER <dl6er@dl6er.de>
DL6ER
force-pushed
the
fix/list-item-query-fallback
branch
from
July 12, 2026 13:28
2d57d57 to
243e6fd
Compare
Member
With this changes here and in pi-hole/web#3818 the |
The `?item=` fallback was only consulted when the routed path left the item empty, so a request carrying both simply ignored the query value. That is safe but silent: nothing told the caller that half of its request was dropped. Parse the query parameter whenever it is present and treat a mismatch as a client error (400). The URI path remains the resource identifier - we never let a query value overrule it, as acting on something else than what a reverse proxy or access log in front of us has seen is worse than refusing the request. Both forms may be given at once only if they name the same item, which keeps every existing client working unchanged. Adds pytest coverage for the three cases: `?item=` alone (a group named `.`), path and query in agreement, and the rejected conflict. Signed-off-by: DL6ER <dl6er@dl6er.de>
Member
Author
|
The web never sends both, so it cannot see the precedence either way - third-party clients can. If the query won, Instead of picking a winner, I made the ambiguity an error: we now parse The form the web uses, Added in 5e5338c together with pytest coverage for all three cases. |
`get_string_var()` ran `mg_url_decode()` over the value `mg_get_var()` returns, but `mg_get_var()` has already decoded it: we leave CivetWeb's `decode_query_string` at its default `no`, so the query string arrives raw and CivetWeb decodes it exactly once when extracting a variable. The second pass changed the value again. `?item=a%2541b` became `aAb` instead of `a%41b`, and any value containing a space or a `+` made `mg_url_decode()` return `-1`, so the parameter silently read as absent. This was harmless enough while `?item=` did not exist, but it is not any more: we now compare the (once-decoded) path item with the (twice-decoded) query item and answer 400 when they disagree, so a client naming a group with a `%` in it got its request rejected. Return what `mg_get_var()` gives us. Variable *names* are still matched against the raw query string, so the percent-encoded names in `queries.c` (e.g. `order%5B0%5D%5Bdir%5D`) keep working. Adds pytest coverage for both cases: a group whose name contains `%` addressed through path and `?item=` at once, and one containing a space addressed through `?item=` alone. Signed-off-by: DL6ER <dl6er@dl6er.de>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: DL6ER <dl6er@dl6er.de>
|
Conflicts have been resolved. |
The fallback added in this branch is a public API surface - `pi-hole/web#3818` uses `PUT /api/groups?item=...` as its regular edit path - but nothing described it. 1. `common.yaml` gains a shared `item` query parameter explaining why it exists (browsers collapse a `.`/`..` path segment) and that it is only consulted when the URI path carries no item, plus an `item_required` variant for the routes where nothing else identifies the resource. 2. `groups`, `clients`, `lists` and `domains` reference it on the operations that take an item, and gain `PUT`/`DELETE` on the collection URI (`/api/groups`, `/api/domains/{type}/{kind}`, ...) - the form the web actually sends. The new operations reuse the existing ones through a YAML anchor, so only the description, the parameters and the `operationId` differ. 3. Each of the four specs gains an `item_mismatch` example for the `400` returned when the URI path and the query name different items. `api_endpoints()` builds FTL's endpoint list from the static table in `api.c`, and `test_all_endpoints_cross_check` compares that table against the specs in both directions, so the argument-less rows have to list the two methods as well. That is declarative only: the dispatch loop stops at the first row whose URI matches, which is always the more specific row above. Signed-off-by: DL6ER <dl6er@dl6er.de>
DL6ER
force-pushed
the
fix/list-item-query-fallback
branch
from
August 9, 2026 08:20
3fb44c0 to
ec5017f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this implement/fix?
A domain, regex, or group whose value is literally
.or..cannot be edited or deleted from the web UI. Per the WHATWG URL standard, a path segment equal to./%2e(single-dot) or../%2e%2e(double-dot) is removed while the browser resolves the URL, so such an item is stripped from everyXMLHttpRequest/fetch/ajaxrequest before it is ever sent. Percent-encoding the dot does not help either, as%2eis normalized identically to.(this is why pi-hole/web#3746 had no effect and was closed).Adding (
POST) and batch deletion (POST /api/domains:batchDelete) already carry the item in the JSON body and are unaffected. OnlyPUT(edit) and the non-batchDELETEread the item from the URI path, so those two operations are impossible for.and...This adds a backward-compatible fallback in
api_list(): when the routed path leavesapi->itemempty, we consult an?item=...query parameter, which is not subject to path dot-segment normalization. Normal items keep using the path unchanged and the query value is only read when the path item is empty, so no existing client changes behavior. The fallback sits before the method dispatch, so it coversGET/PUT/DELETEuniformly for every list type (groups, clients, lists, domains), and it mirrors the existing?type=handling a few lines above.Unlike a client-side delete-and-re-add workaround, this preserves
idanddate_added(a true in-place update).The second commit fixes a defect this one exposes:
get_string_var()decoded query values a second time althoughmg_get_var()had already decoded them.?item=a%2541bbecameaAb, and a value with a space or+read as absent. Harmless while nothing compared the two forms - but we do now, and a group named with a%in it was rejected with a 400.How to test the change during review
Create a group named
.in the web UI, change its comment, then delete it again - both were impossible before. The same works via the API alone:Automated:
test/api/test_m_mutations.pycovers?item=alone, path and query in agreement, the rejected conflict, plus a group name containing%and one containing a space.Additional information
Related issue or feature (if applicable): Fixes pi-hole/web#3308
Pull request in docs with documentation (if applicable): N/A
Accompanying web change: pi-hole/web#3818. The two PRs should be merged together.
By submitting this pull request, I confirm the following:
git rebase)src/dnsmasq/. That tree is a verbatim copy of upstream dnsmasq and we do not carry anything in it that deviates from upstream. Fixes have to go through thednsmasq-discussmailing list first, we merge them once they are in dnsmasq master.Checklist:
developmentbranch.