Add navigation throttle to send the X-Geo header on all platforms [chromium/src : main]

0 views

Skip to first unread message

Aviv Kiss (Gerrit)

unread,

10:22 AM (5 hours ago) 10:22 AM

to Andy Paicu, Chromium Metrics Reviews, Rijubrata Bhaumik, Hans Wennborg, Kentaro Hara, Andrew Rayskiy, Simon Hangl, Enterprise Policy Reviews, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, gcasto+w...@chromium.org, arc-review...@google.com, blink-re...@chromium.org, blink-revie...@chromium.org, roblia...@chromium.org, feature-me...@chromium.org, pdf-r...@chromium.org, kuragin+web-ap...@chromium.org, dewitt...@chromium.org, japhet+...@chromium.org, ios-revie...@chromium.org, philli...@chromium.org, wfh+...@chromium.org, print-rev...@chromium.org, devtools...@chromium.org, lize...@chromium.org, marq+...@chromium.org, aixba+wat...@chromium.org, asvitkine...@chromium.org, rmcelra...@chromium.org, blink-rev...@chromium.org, loading-rev...@chromium.org, cros-print...@google.com, yhanada+...@chromium.org, mgiuca...@chromium.org, android-web...@chromium.org, blink-re...@chromium.org, dibyapal+wa...@chromium.org, browser-comp...@chromium.org, blink-...@chromium.org, twifka...@chromium.org, cblume...@chromium.org, dmurph+watc...@chromium.org, lizeb...@chromium.org, ozone-...@chromium.org, hidehik...@chromium.org, blink-re...@chromium.org, loyso...@chromium.org, hayato...@chromium.org, kinuko...@chromium.org, mac-r...@chromium.org, ios-r...@chromium.org, webap...@microsoft.com, mfoltz+wa...@chromium.org, gavinp...@chromium.org, zelin+watch-we...@chromium.org, devtools-re...@chromium.org, vasilii+watchlis...@chromium.org, net-r...@chromium.org, khmel...@chromium.org, mek+w...@chromium.org, chfreme...@chromium.org, penghuan...@chromium.org, bartek...@chromium.org, extension...@chromium.org, chromium-a...@chromium.org, christia...@chromium.org, jdonnel...@chromium.org, omnibox-...@chromium.org

Attention needed from Andy Paicu

Aviv Kiss added 2 comments

File chrome/browser/omnibox/geolocation_navigation_throttle.h

Line 12, Patchset 8 (Latest):namespace content {}

Aviv Kiss . unresolved

Please fix this WARNING reported by autoreview issue finding: This empty namespace declaration seems to be a leftover and can be removed.

File chrome/browser/omnibox/geolocation_navigation_throttle.cc

Line 67, Patchset 8 (Latest): }

Aviv Kiss . unresolved

Please fix this WARNING reported by autoreview issue finding: If the redirect is to a URL that is NOT a search provider, the 'X-Geo' header added in a previous hop will still be sent because SetRequestHeader persists across redirects. You should call navigation_handle()->RemoveRequestHeader(\"X-Geo\") here to prevent leaking location data to non-search URLs during redirects.

Is there anything to this autoreview finding?

Open in Gerrit

Related details

Attention is currently required from:

Andy Paicu

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Andy Paicu (Gerrit)

unread,

12:41 PM (2 hours ago) 12:41 PM

to Chromium Metrics Reviews, Rijubrata Bhaumik, Hans Wennborg, Kentaro Hara, Andrew Rayskiy, Simon Hangl, Aviv Kiss, Enterprise Policy Reviews, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, gcasto+w...@chromium.org, arc-review...@google.com, blink-re...@chromium.org, blink-revie...@chromium.org, roblia...@chromium.org, feature-me...@chromium.org, pdf-r...@chromium.org, kuragin+web-ap...@chromium.org, dewitt...@chromium.org, japhet+...@chromium.org, ios-revie...@chromium.org, philli...@chromium.org, wfh+...@chromium.org, print-rev...@chromium.org, devtools...@chromium.org, lize...@chromium.org, marq+...@chromium.org, aixba+wat...@chromium.org, asvitkine...@chromium.org, rmcelra...@chromium.org, blink-rev...@chromium.org, loading-rev...@chromium.org, cros-print...@google.com, yhanada+...@chromium.org, mgiuca...@chromium.org, android-web...@chromium.org, blink-re...@chromium.org, dibyapal+wa...@chromium.org, browser-comp...@chromium.org, blink-...@chromium.org, twifka...@chromium.org, cblume...@chromium.org, dmurph+watc...@chromium.org, lizeb...@chromium.org, ozone-...@chromium.org, hidehik...@chromium.org, blink-re...@chromium.org, loyso...@chromium.org, hayato...@chromium.org, kinuko...@chromium.org, mac-r...@chromium.org, ios-r...@chromium.org, webap...@microsoft.com, mfoltz+wa...@chromium.org, gavinp...@chromium.org, zelin+watch-we...@chromium.org, devtools-re...@chromium.org, vasilii+watchlis...@chromium.org, net-r...@chromium.org, khmel...@chromium.org, mek+w...@chromium.org, chfreme...@chromium.org, penghuan...@chromium.org, bartek...@chromium.org, extension...@chromium.org, chromium-a...@chromium.org, christia...@chromium.org, jdonnel...@chromium.org, omnibox-...@chromium.org

Attention needed from Aviv Kiss

Andy Paicu added 2 comments

File chrome/browser/omnibox/geolocation_navigation_throttle.h

Line 12, Patchset 8:namespace content {}

Aviv Kiss . resolved

Please fix this WARNING reported by autoreview issue finding: This empty namespace declaration seems to be a leftover and can be removed.

Andy Paicu

Done

File chrome/browser/omnibox/geolocation_navigation_throttle.cc

Line 67, Patchset 8: }

Aviv Kiss . resolved

Please fix this WARNING reported by autoreview issue finding: If the redirect is to a URL that is NOT a search provider, the 'X-Geo' header added in a previous hop will still be sent because SetRequestHeader persists across redirects. You should call navigation_handle()->RemoveRequestHeader(\"X-Geo\") here to prevent leaking location data to non-search URLs during redirects.
Is there anything to this autoreview finding?

Andy Paicu

Yeah this is a good catch. I added logic to remove the header if it redirects to a non-DSE origin and added some tests.