Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#31468 closed defect (bug) (fixed)

SSL errors/warning when "pressing" an HTTPS URL to PT hosted on an HTTP-only host

Reported by: stephdau's profile stephdau Owned by: azaozz's profile azaozz
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Press This Keywords: has-patch needs-testing
Focuses: Cc:

Description

@helen brought up that trying to press something like a Youtube page, which is SSL by default now, to Press This hosted on a non-SSL URL leads to (at least) Firefox telling the user that they are submitting data to a non-encrypted location (because we use a form POST). The user can allow for it, but it's annoying, as well as leads the new Press This window to lose its forefront focus, hiding it behind the source.

Other browsers let this pass, likely because we're in a bookmarklet context, but still show warning in their consoles.

The proper way to deal with this issue is to not use a form submit in this case. Instead, we should default to passing the basic meta data (title, selection, url), and let PT rely on its server-side media detection instead. This is fine with Firefox and other browsers.

Attached patch does this, as well as use window.focus() when the popup is loaded, for good measure.

Attachments (2)

31468.diff (8.7 KB) - added by stephdau 10 years ago.
31468.2.diff (3.2 KB) - added by azaozz 10 years ago.

Download all attachments as: .zip

Change History (8)

@stephdau
10 years ago

#1 @stephdau
10 years ago

It has to be noted that in some niche contexts, the server-side and client-side parsing (meta data & media detection) might not get the same thing. Youtube would be fine, for example, but technically, the server-side would also get a logged out page, not the one the user sees per se. The Twitter homepage, for another example, would be more problematic, since the user would see their timeline, but the server-side would only get their logged-out homepage.

It also has to be noted that SSL wp-admin is not uncommon. The millions of WordPress.com sites, for example, are all SSL-enabled for wp-admin.

@azaozz
10 years ago

#2 @azaozz
10 years ago

  • Keywords needs-testing added

As fat as I tested this is a problem only in Firefox, at least for now. Perhaps we should limit the fall-back to Firefox only. Would be good to test more in other browsers.

In 31468.2.diff:

  • When protocol mismatch, fall back to server processing only in Firefox.
  • Renamed encodeURI to encURI so it's not confused with the built-in function.
  • Refreshed 31468.diff.

#3 @stephdau
10 years ago

Firefox-only: I was very hesitant about having browser-specific code in there for core, so I abstained. Definitely works for me, if you think that's cool. Chrome, etc, only have console-based warnings, afaik. We used to have a FF-specific test in older PT code, but I thought that for core, we might want to err on the side of not breaking by default if a browser we don't know/expect has a similar issue. But then we'd also never find out about it, since no bug report.

#4 @azaozz
10 years ago

Yeah, better to err on the safe side :) Also just re-tested it and there is a warning in Safari too. Lets go with no browser sniffing.

Last edited 10 years ago by azaozz (previous) (diff)

#5 @stephdau
10 years ago

Sounds like a plan.

#6 @azaozz
10 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 31584:

PressThis: when there is a protocol mismatch (http vs. https), use server-side media detection instead of submitting a form as it triggers "Unsafe data" warning in some browsers. Props stephdau. Fixes #31468.

Note: See TracTickets for help on using tickets.