-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Add strict option for http url in asset #22510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* | ||
* @return array | ||
*/ | ||
protected function splitBaseUrl(array $urls, $isStrictHttp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if ($sslUrls && $baseUrls !== $sslUrls) { | ||
$this->sslPackage = new self($sslUrls, $versionStrategy); | ||
$urlList = $this->splitBaseUrl($this->baseUrls, $isStrictHttp); | ||
if (!empty($urlList['httpsUrl']) && $this->getContext()->isSecure()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The context is stateful, you must check isSecure
in getBaseUrl
, not in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
array(false, array('http://example.com', 'http://example.net', 'https://example.com', 'https://example.net'), '', 'fooa', 'http://example.net/fooa?v1'), | ||
|
||
array(true, array('http://example.com', 'http://example.net', 'https://example.com', 'https://example.net'), '', 'foo', 'https://example.com/foo?v1'), | ||
array(true, array('http://example.com', 'http://example.net', 'https://example.com', 'https://example.net'), '', 'fooa', 'https://example.net/fooa?v1'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a test case with a protocol-relative URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
*/ | ||
private function splitBaseUrl(array $urls, $isStrictHttp) | ||
{ | ||
$urlList = array('httpUrl' => array(), 'httpsUrl' => array(), 'fullUrl' => array()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this array of arrays, having 2 private properties baseSecureUrls
and baseUnsecureUrls
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@RedBoool thanks for contributing this feature! I'd like to ask you a question: Is really a technical issue to use HTTPS on HTTP pages? Will browsers display any warning message or will it affect your application or your end users in any other way? I know it's an issue the other way (using HTTP on HTTPS pages).
What do others think? |
This is not a technical issue, it's a financial issue. The aim of this PR is to help us pay the right price, no more, no less |
@RedBoool would that fit your request? ping @symfony/deciders would that be OK to you? |
That perfectly match my needs. |
It makes me a bit nervous... if your protocol is |
Moving to 4.1 as it looks like the discussion is not over. |
Maybe it's time to rethink this feature proposal. HTTPS is no longer optional for the web. Google and others are pushing hard to make it mandatory. According to Google's data (https://transparencyreport.google.com/https/overview) more than 70% of web traffic is now encrypted. So I guess CDNs charging more for HTTPS traffic will be soon out of the market. |
👎 on my side. Mixed content should be avoided, and using HTTPS everywhere is a best practice we must encourage. |
Uh oh!
There was an error while loading. Please reload this page.