Issue metadata
Sign in to add a comment
|
Navigation entry's SSL status is not updated when navigating to an existing page |
||||||||||||||||||||||||
Issue descriptionWhen we commit a navigation to an existing page, we only update the navigation entry's SSLStatus when we're restoring an entry, or when there was a pending entry that got lost [1]. Otherwise, we don't update the SSL status. This means that the navigation entry's SSL status doesn't reflect the actual navigation that happened, but rather the request with which the existing entry was loaded. I can repro this reliably with a server that alternates between serving a 200 and a 301 to google.com: import SimpleHTTPServer import SocketServer i = 0 class myHandler(SimpleHTTPServer.SimpleHTTPRequestHandler): def do_GET(self): global i print self.path if self.path == "/favicon.ico": self.send_response(404) self.end_headers() return i = i + 1 if i % 2 == 1: self.send_response(200) self.end_headers() return self.send_response(301) new_path = '%s%s'%('https://google.com', self.path) self.send_header('Location', new_path) self.end_headers() PORT = 8000 handler = SocketServer.TCPServer(("", PORT), myHandler) print "serving at port 8000" handler.serve_forever() If you run that script, visit http://localhost:8000, and then refresh, you get redirected to google.com and it is missing its lock icon. Filing this as a security bug because I think it's possible that this could end up with showing a Secure chip on a broken-https page or other weirdnesses, not just losing the lock icon. [1] https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl.cc?dr=CSs&l=1250
,
Dec 6 2017
Interestingly, in Chrome 65 we get a state where PageInfo thinks the page is secure, but the security chip still shows (i). https://bayden.com/test/maybeEric.asp (refresh until repro appears).
,
Dec 6 2017
Thanks Emily for the repro. I'll take a look.
,
Dec 7 2017
,
Dec 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f24708ee85f482bb53f14cb6e1ce22e2a809855 commit 3f24708ee85f482bb53f14cb6e1ce22e2a809855 Author: John Abd-El-Malek <jam@chromium.org> Date: Thu Dec 07 19:02:19 2017 Fix the SSLState being lost on a NavigationEntry when navigating to an existing insecure page that redirected to a secure page. Bug: 792221 Change-Id: Ia5f364cd370681cc61935652c7737425edcc4e45 Reviewed-on: https://chromium-review.googlesource.com/813054 Reviewed-by: Emily Stark <estark@chromium.org> Commit-Queue: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#522494} [modify] https://crrev.com/3f24708ee85f482bb53f14cb6e1ce22e2a809855/chrome/browser/ssl/ssl_browsertest.cc [modify] https://crrev.com/3f24708ee85f482bb53f14cb6e1ce22e2a809855/content/browser/frame_host/navigation_controller_impl.cc
,
Dec 7 2017
I'll request merge approval tomorrow.
,
Dec 8 2017
,
Dec 9 2017
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 9 2017
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 10 2017
,
Dec 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0dac85296ca1c10afb5623f1135caf6aeb44dfcf commit 0dac85296ca1c10afb5623f1135caf6aeb44dfcf Author: John Abd-El-Malek <jam@chromium.org> Date: Mon Dec 11 16:35:46 2017 Fix the SSLState being lost on a NavigationEntry when navigating to an existing insecure page that redirected to a secure page. Bug: 792221 Change-Id: Ia5f364cd370681cc61935652c7737425edcc4e45 Reviewed-on: https://chromium-review.googlesource.com/813054 Reviewed-by: Emily Stark <estark@chromium.org> Commit-Queue: John Abd-El-Malek <jam@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#522494}(cherry picked from commit 3f24708ee85f482bb53f14cb6e1ce22e2a809855) Reviewed-on: https://chromium-review.googlesource.com/819850 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#130} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/0dac85296ca1c10afb5623f1135caf6aeb44dfcf/chrome/browser/ssl/ssl_browsertest.cc [modify] https://crrev.com/0dac85296ca1c10afb5623f1135caf6aeb44dfcf/content/browser/frame_host/navigation_controller_impl.cc
,
Jan 22 2018
,
Mar 17 2018
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 27 2018
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by creis@chromium.org
, Dec 6 2017