Closed Bug 1884941 Opened 11 months ago Closed 11 months ago

Enhanced Tracking Protection level "Strict" urldecodes url parameters

Categories

(Core :: Privacy: Anti-Tracking, defect, P1)

Firefox 123
defect

Tracking

()

VERIFIED FIXED
126 Branch
Tracking Status
firefox-esr115 --- wontfix
firefox124 --- wontfix
firefox125 --- verified
firefox126 --- verified

People

(Reporter: bugzilla, Assigned: abhishekmadan)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:123.0) Gecko/20100101 Firefox/123.0

Steps to reproduce:

On nightly the bug is present regardless of what ETP setting is selected.

https://httpbin.org/redirect-to?url=https%3A%2F%2Fexample.com%3Fa%3Db%26c%3Dd (no mc_eid=f20cd58c07) goes to the correct location no matter what.

This was originally found because it was breaking a real tracking url in a newsletter that was supposed to end up at https://www.savannahga.gov/AgendaCenter/Search/?term=&CIDs=5,&startDate=&endDate but was instead going to https://www.savannahga.gov/AgendaCenter/Search/?term=

Actual results:

When visiting ttps://httpbin.org/redirect-to?url=https%3A%2F%2Fexample.com%3Fa%3Db%26c%3Dd&mc_eid=f20cd58c07 FF parses the string, strips the mc_eid param, and makes a request to https://httpbin.org/redirect-to?url=https://example.com?a=b&c=d (note the url encoding has been lost). This cases httpbin.org to grab c=d as a parameter to itself and just redirect to https://example.com?a=b instead.

Expected results:

Firefox should have stripped the &mc_eid=f20cd58c07 part but made a request to https://httpbin.org/redirect-to?url=https%3A%2F%2Fexample.com%3Fa%3Db%26c%3Dd and had the user end up on https://example.com/?a=b&c=d

The Bugbug bot thinks this bug should belong to the 'Core::Privacy: Anti-Tracking' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Privacy: Anti-Tracking
Product: Firefox → Core
Severity: -- → S3
Priority: -- → P3
Assignee: nobody → amadan
Attachment #9391754 - Attachment description: WIP: Bug 1884941 - added an option to prevent decoding to URLParams. r=#anti-tracking-reviewers → Bug 1884941 - added an option to prevent decoding to URLParams. r=#anti-tracking-reviewers
Attachment #9391754 - Attachment description: Bug 1884941 - added an option to prevent decoding to URLParams. r=#anti-tracking-reviewers → Bug 1884941 - Add an option to URLParams::Parse() for if it should decode the parameters or not. r=#anti-tracking-reviewers
Attachment #9391754 - Attachment description: Bug 1884941 - Add an option to URLParams::Parse() for if it should decode the parameters or not. r=#anti-tracking-reviewers → Bug 1884941 - added an option to prevent decoding to URLParams. r=#anti-tracking-reviewers
Attachment #9391754 - Attachment description: Bug 1884941 - added an option to prevent decoding to URLParams. r=#anti-tracking-reviewers → Bug 1884941 - Add an option to URLParams::Parse() for if it should decode the parameters or not. r=#anti-tracking-reviewers.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: P3 → P1

Abhishek, could you please mark all the affected versions? Potentially this is a regression and we should find the bug. Mozregression could help here. You can force enable QPS via pref when using mozregression.

Also we need test coverage for this bug. The current tests don't seem to catch it.

Thanks!

Flags: needinfo?(amadan)

This has been broken since Fx96, this is patch that cause the issue https://hg.mozilla.org/mozilla-central/rev/76ba18f48c8b46d3bca3ec167fc1bbdd803177e9.

I have filed a separate bug for adding test cases for this case.

Flags: needinfo?(amadan)
Pushed by amadan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/45931e6db21e Add an option to URLParams::Parse() for if it should decode the parameters or not. r=anti-tracking-reviewers,necko-reviewers,valentin,timhuang.
Blocks: 1887742
Pushed by amadan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/78436da20d95 Add an option to URLParams::Parse() for if it should decode the parameters or not. r=anti-tracking-reviewers,necko-reviewers,valentin,timhuang.

Backed out for causing non unified build bustages on URLQueryStringStripper.cpp.

[task 2024-03-26T17:28:38.446Z] 17:28:38     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/toolkit/components/antitracking'
[task 2024-03-26T17:28:38.449Z] 17:28:38     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -o URLQueryStringStripper.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DNDEBUG=1 -DTRIMMED=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/toolkit/components/antitracking -I/builds/worker/workspace/obj-build/toolkit/components/antitracking -I/builds/worker/checkouts/gecko/netwerk/base -I/builds/worker/checkouts/gecko/netwerk/protocol/http -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtautological-constant-in-range-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wenum-compare-conditional -Wenum-float-conversion -Wno-error=deprecated -Wno-error=deprecated-anon-enum-enum-conversion -Wno-error=deprecated-enum-enum-conversion -Wno-error=deprecated-pragma -Wno-error=deprecated-this-capture -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wformat -Wformat-security -Wno-psabi -Wthread-safety -Wno-error=builtin-macro-redefined -Wno-unknown-warning-option -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/URLQueryStringStripper.o.pp   /builds/worker/checkouts/gecko/toolkit/components/antitracking/URLQueryStringStripper.cpp
[task 2024-03-26T17:28:38.449Z] 17:28:38    ERROR -  /builds/worker/checkouts/gecko/toolkit/components/antitracking/URLQueryStringStripper.cpp:445:11: error: use of undeclared identifier 'LOG'
[task 2024-03-26T17:28:38.450Z] 17:28:38     INFO -    445 |           LOG(("Unable to obtain ioService"));
[task 2024-03-26T17:28:38.450Z] 17:28:38     INFO -        |           ^
[task 2024-03-26T17:28:38.450Z] 17:28:38     INFO -  1 error generated.
[task 2024-03-26T17:28:38.450Z] 17:28:38    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:688: URLQueryStringStripper.o] Error 1
[task 2024-03-26T17:28:38.450Z] 17:28:38     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/toolkit/components/antitracking'
Flags: needinfo?(amadan)
Flags: needinfo?(amadan)
Pushed by amadan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ec5346caf598 Add an option to URLParams::Parse() for if it should decode the parameters or not. r=anti-tracking-reviewers,necko-reviewers,valentin,timhuang.
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 126 Branch

The patch landed in nightly and beta is affected.
:abhishekmadan, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox125 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(amadan)
Flags: needinfo?(amadan)

Comment on attachment 9391754 [details]
Bug 1884941 - Add an option to URLParams::Parse() for if it should decode the parameters or not. r=#anti-tracking-reviewers.

Beta/Release Uplift Approval Request

  • User impact if declined: Any redirect links that contain a query parameter that is stripped by QPS would also strip all but the first query parameter for the redirect link which could cause navigational breakage.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The only modification made was to add an option to prevent decoding of query parameters to URL Parse function
  • String changes made/needed:
  • Is Android affected?: Yes
Attachment #9391754 - Flags: approval-mozilla-beta?

Where are the automated tests for this?

Flags: needinfo?(amadan)

Bug 1887742 is more along the lines of what I was asking about. Please note that Phabricator asks for a comment to be added when setting testing-exception-elsewhere on a revision pointing to where that coverage elsewhere is.

Comment on attachment 9391754 [details]
Bug 1884941 - Add an option to URLParams::Parse() for if it should decode the parameters or not. r=#anti-tracking-reviewers.

Approved for 125.0b9.

Attachment #9391754 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

I have reproduced this issue on an affected Nightly build, 2024-03-12, using STR from comment 0.

The issue is verified as fixed on Beta 125.0b9 and latest Nightly 126.0a1 under macOS 13, Ubuntu 22.04 and Win 11.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: