To allow Ajax requests from a different domain and take advantages of the new REST module more globally.

W3C Draft: http://www.w3.org/TR/cors/

What: http://en.wikipedia.org/wiki/Cross-origin_resource_sharing
Why: http://annevankesteren.nl/2012/12/cors-101
Who: http://caniuse.com/cors
How: Via https://github.com/asm89/stack-cors library

This header needs to be set everywhere (or pretty much everywhere).
Access-Control-Allow-Origin: *

Might be a REST module thing but putting on base system just in case.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority This is highly important to headless setups (Angular, Ember, etc.) , which often run on a different domain than their backend. Without CORS they cannot actually talk to Drupal. Given that this seems major.
Issue category Feature request: REST applications can still be built as native apps or javascript apps on the same domain, so this is not a bug; rather, it is an expansion of functionality.
Prioritized changes Not a prioritized change for 8.0.x.
Disruption There is no impact on existing developers. No APIs change as a result of this issue; we only add some more (disabled by default) properties to default.settings.yml
CommentFileSizeAuthor
#152 interdiff.txt828 bytesslasher13
#152 1869548-152.patch9.23 KBslasher13
#150 interdiff.txt2.69 KBslasher13
#150 1869548-150.patch9.92 KBslasher13
#142 1869548-141.patch9.22 KBalexpott
#142 130-141-interdiff.txt6.85 KBalexpott
#130 opt_in_cors_support-1869548-130.patch15.95 KBchr.fritsch
#110 1869548-110.patch15.93 KBdawehner
#106 interdiff.txt1.37 KBdawehner
#106 1869548-106.patch9.51 KBdawehner
#104 interdiff.txt2.7 KBdawehner
#104 1869548-104.patch9.35 KBdawehner
#102 1869548-102.patch37.13 KBdawehner
#96 1869548-96.patch38.24 KBdawehner
#94 1869548-94.patch38.24 KBdawehner
#90 1869548-90.patch36.13 KBAlxVallejo
#87 1869548-87.patch38.24 KBdawehner
#85 enable_cors-1869548-83.patch36.24 KBjeqq
#81 enable_cors-1869548-81.patch37.93 KBjeqq
#65 1869548-63-without-vendor-do-not-test.patch6.4 KBeffulgentsia
#63 interdiff.txt2.1 KBdawehner
#63 1869548-63.patch36.51 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,431 pass(es). View
#60 1869548-60.patch36.49 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,369 pass(es). View
#58 interdiff.txt1.11 KBdawehner
#58 1869548-58.patch38.24 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,210 pass(es). View
#56 interdiff.txt4.86 KBdawehner
#56 1869548-56.patch38.15 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,209 pass(es), 2 fail(s), and 0 exception(s). View
#49 1869548-49.patch34.3 KBjeqq
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,980 pass(es). View
#47 interdiff.txt3.54 KBdawehner
#47 1869548-47.patch34.5 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#46 1869548-46.patch31.34 KBdixon_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,043 pass(es). View
#39 1869548-39.patch31.74 KBdamiankloip
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,184 pass(es). View
#35 iinterdiff.patch1.62 KBdmouse
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch iinterdiff.patch. Unable to apply patch. See the log in the details link for more information. View
#35 cors-1869548-35.patch32.44 KBdmouse
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,250 pass(es). View
#32 1869548-32.patch32.83 KBdmouse
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1869548-32.patch. Unable to apply patch. See the log in the details link for more information. View
#27 1869548-27.patch31.74 KBdamiankloip
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,832 pass(es). View
#26 core-cors-headers-1869548-26.patch790 bytes-enzo-
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 42,842 pass(es), 20,370 fail(s), and 1,397 exception(s). View
#20 core-cors-headers-1869548-20.patch1.38 KByanniboi
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,170 pass(es), 14 fail(s), and 0 exception(s). View
#8 core-cors-headers-1869548-8.patch1.3 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 49,382 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

nod_’s picture

Issue summary: View changes

add W3C link

nod_’s picture

Issue tags: +WSCCI

tagging

Crell’s picture

Component: base system » rest.module
Issue tags: +needs security review

I think this is a REST module thing in particular.

Anything that says "Allow: *" automatically makes me nervous from a security perspective. I'm not fully versed in CORS so I don't know if that's warranted, but tagging for the security team to get their opinion.

Damien Tournoud’s picture

Status: Active » Postponed

That makes total sense, but only makes sense from a security perspective if you use for those requests an authentication mechanism that is separate from the one used by the standard browser-based requests. For example, it works well for Facebook / Google / Twitter using the oAuth2 "Implicit grant" authorization flow.

It will not work from a security perspective if we cannot mandate cross-domain requests to use a different authentication mechanism.

This needs to be postponed until:

  • We have any other authentication mechanism in core that is useful for cross-domain REST requests (oAuth2 implicit grant is what the big boys use, so we should aim at one);
  • We have useful authorization mechanisms for the REST endpoints, we do not need to have full-fledges scopes, but at the minimum the REST endpoints need to respect field-level access control.
nod_’s picture

Well I was just thinking about GET. What I had in mind was adding the header to all the entity that one can GET anonymously through the REST module. That'd be a start.

(edit) the HTTP methods used can be restricted with Access-Control-Allow-Methods. I'll try to see if I understand enough of Symfony to roll a quick & dirty patch.

Damien Tournoud’s picture

Allowing GET while still respecting cookie authentication will open the site to information disclosure. That's just not acceptable.

nod_’s picture

Access-Control-Allow-Credentials can be set the omit credentials flag can be set so that cookies and HTTP auth will be ignored. A patch should help I'll try to find someone to walk me through a couple of things.

Damien Tournoud’s picture

Title: Enable CORS for everything* by default » Enable CORS for GET requests
Status: Postponed » Active

Access-Control-Allow-Credentials defaults to false, so this just returning Access-Control-Allow-Origin: * to GET requests seems sufficient.

nod_’s picture

FileSize
1.3 KB
PASSED: [[SimpleTest]]: [MySQL] 49,382 pass(es). View

An embarrassing patch that adds the headers to all pages, regardless of which page it is.

Needs to be improved a lot, that's as far as my symfony knowledge goes. Need to check the route so that we're in one that the REST module is responsible for.

andypost’s picture

Status: Active » Needs review

Implementation looks good, suppose we needs a unit test for that

Damien Tournoud’s picture

Status: Needs review » Needs work

#8 is just a first attempt :) We only want the header for the REST module routes.

Wim Leers’s picture

The CDN module does this automatically for publicly available static files. (Necessary to e.g. make fonts in Firefox work if they're hosted on a different domain.) My experience is limited to that extremely simple case, so I don't think I can contribute much to this discussion.

linclark’s picture

Regarding #6-#7, if the wildcard is used, the Access-Control-Allow-Credentials is restricted to false. That is to say, even if it is set to true by another event subscriber, that will be ignored because the wildcard prevents credentials from being included on cross-origin requests (see Hacking Web Apps section on HTML5).

Not necessarily a problem for us, but just wanted to make sure it was noted here.

Damien Tournoud’s picture

@linclark: yes, Access-Control-Allow-Credentials being false is both by design and desirable.

Just an additional note here: we also need to explicitly allow some (response) headers with Access-Control-Expose-Headers. (At the minimum, the Location header.)

Also, the CORS implementation of most if not all of the current browsers is severely broken in many fun and interesting ways.

linclark’s picture

I wasn't saying that we would design it to be false, but rather that this is a restriction outside our control if we use the wildcard.

If someone does want to enable it to be true, they would have to unsubscribe this subscriber and add their own which determines the Origins to allow. This seems reasonable.

Wim Leers’s picture

#13: links for interesting & fun brokenness? :)

linclark’s picture

Also, RE: #10:

We only want the header for the REST module routes.

Is there any security reason for doing this? In Hacking Web Apps, it says:

Remember that CORS establishes sharing on a per-origin basis, not a per-resource basis. If it is only necessary to share a single resource, consider moving that resource to its own subdomain rather than exposing the rest of the web application’s resources. For example, establish a separate origin for API access rather than exposing the API via a directory on the site’s main origin.

I looked at the resource sharing check algorithm in the spec and it seemed to me like it was on a per-resource basis. Additionally, Anne VK's response in a mailing list thread seems to indicate it is per-resource. However, I did find an RFC which seems to indicate the same thing as Hacking Web Apps. From RFC 6454:

When adding new functionality to the web platform, it can be tempting to grant a privilege to one resource but to withhold that privilege from another resource in the same origin. However, withholding privileges in this way is ineffective because the resource without the privilege can usually obtain the privilege anyway because user agents do not isolate resources within an origin. Instead, privileges should be granted or withheld from origins as a whole (rather than discriminating between individual resources within an origin) [BOFGO].

So if we depend on per-resource differentiation (e.g. only adding the header to REST module routes) for security purposes, it seems like we'd be going against best practice. However, it might make sense to do it for another reason.

linclark’s picture

Issue summary: View changes

link fail

kim.pepper’s picture

Issue summary: View changes

This may be related to CORS: Chrome needs to make preflight requests using OPTIONS http verb, but I can't see any support for OPTIONS in the rest module.

kim.pepper’s picture

kim.pepper’s picture

I've ported the contrib CORS module to D8 over here: https://github.com/previousnext/drupal_cors

yanniboi’s picture

FileSize
1.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,170 pass(es), 14 fail(s), and 0 exception(s). View

I'm building a production site in d8 (I know I'm crazy ;P ) and I need CORS, so this is what I have so far...

Please comment!

andypost’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
@@ -43,4 +46,19 @@ public function onAlterRoutes(RouteBuildEvent $event) {
+    watchdog('rest', 'REST CORS ALLOWED!! 2');

deprecated

yanniboi’s picture

Oh snap! I was using that for debugging. That shouldn't be in there!

nod_’s picture

Adding realted issue. Might be possible to implement this with https://github.com/asm89/stack-cors

dawehner’s picture

@yanniboi, kim.pepper
Your implementation sure work due to the flexibility of the kernel events.
Using the stack approach would thought have a couple of advantages:

kim.pepper’s picture

@dawehner. Looks good. I'd like to know whether there is support for getting this into core before I'd start integrating with the contrib CORS module.

-enzo-’s picture

FileSize
790 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 42,842 pass(es), 20,370 fail(s), and 1,397 exception(s). View

I'm creating a new version of my library https://github.com/enzolutions/backbone.drupal to enable use Headless Drupal using front end in a different frontend domain.

The problem I found with this kind of CORS request explained about is related with how jQuery Ajax execute the first request, because before to do a GET|PUT|DELETE|POST|PATCH request an OPTIONS request is executed to validate request methods accepted for backend server[1].

The current status of Rest module doesn't implement the OPTIONS, so my first approach was implement that method in class Drupal\rest\Plugin\rest\resource\EntityResource using the following code

public function options(EntityInterface $entity) {
   $response = new ResourceResponse();
   $response->setStatusCode(ResourceResponse::HTTP_OK);
   $response->headers->set('Allow', 'GET,POST,PATCH,DELETE,OPTIONS');
   return $response->send();
  }

After that extra function in class EntityResource the method was available and double checked with module RestUI https://www.drupal.org/project/restui, but this solution wasn't enough because I can't enable OPTIONS method without authentication and jQuery trigger the method without CRC or Basic Auth method even if you provide in call ( I will create a issue in RestUI to request the option to enable a method without authentication).

So to resolve I follow the recommendation of @dawehner in comment #24 to use .htacess to enable CORS and intercept the OPTIONS request to avoid Drupal reject the request because doesn't have the proper Basic Auth credentials, the patch is attached but let me list the changes.

1. Rule to avoid Drupal process the options request

# Intercept OPTIONS calls
RewriteCond %{REQUEST_METHOD} OPTIONS
RewriteRule .* / [R=200,L]

2. Rules to enable CORS and send the proper headers even if interception occurs

Header always set Access-Control-Allow-Origin "*"
Header always set Access-Control-Allow-Methods "POST, GET, OPTIONS, PATCH, DELETE"
Header always set Access-Control-Allow-Headers: Authorization

I recommend to change * for your frontend domain i.e frontend-example.com

[1] http://api.jquery.com/jquery.ajax/

damiankloip’s picture

Status: Needs work » Needs review
FileSize
31.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,832 pass(es). View

We can implement this in stack by just pulling in the stack library and adding a few lines to our core.services.yml:

  http_middleware.cors:
    class: Asm89\Stack\Cors
    arguments: [{ allowedHeaders: [*], allowedMethods: ['DELETE', 'GET', 'POST', 'PUT'], allowedOrigins: [*] }]
    tags:
      - { name: http_middleware }

If we can do this, it's more preferable to an htaccess solution? Although no doubt that works. Also, any of those parameters could be easily overridden.

Crell’s picture

Part of the question, I think, is whether we want this enabled by default or just easily enabled. And if easily enabled, how?

dawehner’s picture

It would be great if someone who actually deal with headless drupal could test whether this patch works. On top of that some basic integration test would be cool, so that we really know that CORS is running.

+++ b/core/core.services.yml
@@ -394,6 +394,11 @@ services:
+    tags:
+      - { name: http_middleware }

so do we really want to run this after page caching? just thinking loud

nod_’s picture

by default enabled.

dawehner’s picture

If we can do this, it's more preferable to an htaccess solution? Although no doubt that works. Also, any of those parameters could be easily overridden.

Well even this is really nice, having it on .htaccess itself is much faster, if you are honest but well you can still
replace the middleware and use apache, if you know what you do.

dmouse’s picture

FileSize
32.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1869548-32.patch. Unable to apply patch. See the log in the details link for more information. View

Hi, I change this patch to create a wrapper to create an instance of Cors class.

Status: Needs review » Needs work

The last submitted patch, 32: 1869548-32.patch, failed testing.

damiankloip’s picture

Do you have an interdiff? and why do we need that? It seems the Drupal namespaced Cors wrapper has no benefit at all?

dmouse’s picture

FileSize
32.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,250 pass(es). View
1.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch iinterdiff.patch. Unable to apply patch. See the log in the details link for more information. View

The wrapper is necessary for libraries that are implemented if they change, you can also put a more level configuration, an interface that allows you to manage the domains that can access your content.

dawehner’s picture

I'm sorry but we don't wrap exteral libraries like that atm.
You could easily put this domain configuration into the container as well, no need for another class.

damiankloip’s picture

Also, our dependencies would not get just get updated if there was such a big change. If that was the case it would be broken either way.

dawehner’s picture

@dmouse
On top of that, by definition stack implementations have one inderface, that will not change.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
31.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,184 pass(es). View

Right.

@dmouse, any work that allow this to be configurable should be a follow up issue.

Here is a reroll of #27.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@dmouse, any work that allow this to be configurable should be a follow up issue.

Agreed. If you can come up with good usecase, sure, try to use one.

The last submitted patch, 26: core-cors-headers-1869548-26.patch, failed testing.

The last submitted patch, 35: iinterdiff.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/core.services.yml
@@ -394,6 +394,11 @@ services:
+    arguments: [{ allowedHeaders: [*], allowedMethods: ['DELETE', 'GET', 'POST', 'PUT'], allowedOrigins: [*] }]

I really don't think that allowing all origins should be the default behaviour. I would expect some discussion of security on this issue and considering the nature of the change and the fact we are enabling it by default I would expect a change record.

The last submitted patch, 20: core-cors-headers-1869548-20.patch, failed testing.

Crell’s picture

I would agree that default-allow-world is a bad idea. So how do we want to enable allowing certain origins, and/or all origins? Ie, what's the API we want to expose for that?

Options I see are:

1) Edit .htaccess
2) Edit services.yml (can we do it there?)
3) Write a compiler pass

If we can make it work option 2 would be my preference.

dixon_’s picture

Status: Needs work » Needs review
FileSize
31.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,043 pass(es). View

Plain re-roll of the patch from #39.

dawehner’s picture

FileSize
34.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
3.54 KB

Tried to a) add test coverage b) does solution 2)

Status: Needs review » Needs work

The last submitted patch, 47: 1869548-47.patch, failed testing.

jeqq’s picture

Status: Needs work » Needs review
FileSize
34.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,980 pass(es). View

Updated the patch to work on latest Drupal 8 HEAD, fixed the broken tests.

Crell’s picture

Before we keep rolling patches, does anyone have an opinion on #45? What patches do we even want to be rolling here?

ygerasimov’s picture

I would vote for possibility to control these headers per route. For example by adding special tag or something to the route and then this route will get all headers according to configuration of cors.

dixon_’s picture

@Crell I would definitely vote for option 2. It makes total sense to configure generic components with container parameters. This way you can override the settings both manually and modules can do it automatically, by writing their own compiler pass.

@ygerasimov I like that idea. But this is something that contrib could implement with a special compiler pass. I would vote for this initial core implementation to be simple and straight forward, e.g. just using one central container parameter like the patch does now.

dawehner’s picture

I really don't think that allowing all origins should be the default behaviour. I would expect some discussion of security on this issue and considering the nature of the change and the fact we are enabling it by default I would expect a change record.

Just to be clear, the new default is basically the default behaviour without CORS. Maybe we should look inside settings.php.
In case there is a $base_urlset we could allow that by default?

Crell’s picture

If we use the Stack lib, then the services.yml approach would likely involve setting a parameter that is checked by a compiler pass, which will then register/not register the middleware and configure it. Eg, a parameter that specifies what the legal origins are. If none or not set, then just don't enable the middleware.

Alex, would that be sufficiently tight, security-wise?

alexpott’s picture

re #54 that sounds fine

dawehner’s picture

FileSize
38.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,209 pass(es), 2 fail(s), and 0 exception(s). View
4.86 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, 56: 1869548-56.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,210 pass(es). View
1.11 KB

There we go.

Crell’s picture

Assigned: Unassigned » Dries
Issue summary: View changes
  1. +++ b/sites/default/default.services.yml
    @@ -51,3 +51,20 @@ parameters:
    +  # Allow to configure Cross-Site HTTP requests (CORS).
    

    Ah, German idioms. :-) Try just "Configure Cross-Site HTTP requests (CORS)"

  2. +++ b/sites/default/default.services.yml
    @@ -51,3 +51,20 @@ parameters:
    +  # Note: By default the configuration is done in a way that no behaviour is
    +  # changed.
    

    I don't understand this sentence. Do you mean "by default CORS is disabled"? (That's what it looks like from line 62.)

Other than those doc points I think this is good.

Also assigning to Dries for when it gets RTBCed; it has a new library so he has to commit it. Updating summary for beta considerations.

dawehner’s picture

Priority: Normal » Major
Issue summary: View changes
FileSize
36.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,369 pass(es). View

Rerolled and adapted for the docs suggestions of @Crell (yeah there has been some grammer lacks here)

Crell’s picture

Didn't fix the #59.1.

damiankloip’s picture

+++ b/core/modules/system/src/Tests/HttpKernel/CorsIntegrationTest.php
@@ -0,0 +1,56 @@
+    $this->assertEqual([], $cors_config['allowedHeaders']);
+    $this->assertEqual([], $cors_config['allowedMethods']);
...
+    $this->assertEqual(FALSE, $cors_config['allowedHeaders']);
+    $this->assertEqual(FALSE, $cors_config['allowedMethods']);

This seems to just be asserting the same thing twice.

[] == false is true
false == false is true

Do you want to just remove the second lot and use assertIdentical on the first? Actually maybe assertIdentical on all the options there?

dawehner’s picture

FileSize
36.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,431 pass(es). View
2.1 KB

Adressed both points.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Gift for the headless folks...

effulgentsia’s picture

Looks like security was addressed in #7 and #55. Here's also a -do-not-test patch sans core/vendor and composer.lock for easier review of the changes to Drupal.

effulgentsia’s picture

Issue summary: View changes

Added link to the https://github.com/asm89/stack-cors library in the issue summary.

Dries’s picture

Version: 8.0.x-dev » 8.1.x-dev
Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Postponed

I think this is valuable, but it is ultimately a feature request. It's not a bug, because REST applications can still be built as native apps or javascript apps on the same domain. We're not adding new features at this point of the release so I'm postponing this to 8.1.x. Would love to see it in 8.1.x though. In the meantime, this can work perfectly well as a contributed module, right?

xjm’s picture

Issue summary: View changes

Updating the summary based on #67.

alexpott’s picture

There is also apparently another method not involving a module... http://enzolutions.com/articles/2014/09/08/how-to-enable-cors-requests-a...

dawehner’s picture

I think this is valuable, but it is ultimately a feature request. It's not a bug, because REST applications can still be built as native apps or javascript apps on the same domain. We're not adding new features at this point of the release so I'm postponing this to 8.1.x. Would love to see it in 8.1.x though. In the meantime, this can work perfectly well as a contributed module, right?

Fair, hopefully someone does it.

Its just sad because that issue would not had any disruption.

chr.fritsch’s picture

I've created a module for that. Any improvments are welcome

https://www.drupal.org/sandbox/chr.fritsch/2460591

clemens.tolboom’s picture

@chr.fritsch there is already a module https://www.drupal.org/project/cors with a pending #2328985: Drupal 8 port for CORS

chr.fritsch’s picture

Thanks for the info

decibel.places’s picture

I am interested in this because I am writing a book about CORS, including a chapter about using CORS in popular CMSs.

Here are some thoughts.

jQuery Supports Limited CORS

Drupal 8 already supports CORS via jQuery in core/assets/jquery/jquery.js|jquery.min.js
support.cors = !!xhrSupported && ( "withCredentials" in xhrSupported );

However, CORS via jQuery is very limited, it does not support preflight without additional custom code. Preflight is often needed for requests that are not simple, and can improve security.

Adding CORS Headers With the CORS Module

The CORS module is a handy way to map headers required for CORS to Drupal paths. I think its most appropriate use is when integrating a service or API that requires addition of CORS headers, without additional custom coding.

An interesting feature of the CORS module is the <mirror> token that will add the domain from a request that sends the Origin header to the Access-Control-Allow-Origin header. This is like a slightly limited use of the wildcard, because requests without the Origin header will not be allowed.

CORS Should Be Implemented With Custom Code

In most cases, CORS should be implemented very carefully and with additional logic to validate and process the requests, including preflight when needed. This assumes custom code, probably in a custom module. In a custom implementation, the headers should be managed in that code, which makes the CORS module unnecessary for these cases.

Limited Support for Multiple Domains in the Access-Control-Allow-Origin Header

The W3C specification defines a space-separated list of allowed domains. However, multiple allowed domains are not yet supported in all clients.

That is the reason many people suggest using the wildcard "*" but that allows requests from ALL domains. For security additional processing of requests, and perhaps preflight, are needed when using the wildcard.

Recommendations for Including the Access-Control-Allow-Origin Header in Drupal Core

Therefore if the Access-Control-Allow-Origin header is to be added in Drupal core, I recommend:

  1. The header is not added by default, it should be a configuration option.
  2. As part of the configuration option, the allowed domain(s) could be configured.
  3. Rather than adding the CORS header(s) globally, mapping to paths similar to the function in the CORS module should be considered.
  4. If the wildcard is used, there should be a warning about the risks, and link(s) to documentation for security best practices in CORS.
  5. If the wildcard is used globally, then any additional header added for a specific path will trigger a warning about trying to send additional headers. A message or warning should accompany configuration with the wildcard saying additional headers can not be set. (BTW, This warning can also be triggered by improper configuration of the CORS module.)
  6. Consider that Access-Control-Allow-Origin is not the only header needed for any but the most simple uses of CORS. The CORS module can also add additional headers:
    1. Access-Control-Allow-Methods
    2. Access-Control-Allow-Headers
    3. Access-Control-Allow-Credentials

    For a complete implementation of CORS in Drupal core, configuration adding additional headers when needed should be considered.

What About JSON-P?

In some cases cross-origin requests may be implemented with JSON-P. It is much simpler, and less secure, than using CORS.

pkosenko’s picture

There is a Drupal 7 CORS custom module that doesn't look like it is being assiduously maintained or updated to Drupal 8 on the module site.

However, someone else on Github has contributed an update for Drupal 8. I would suggest that it be posted over to the DRUPAL archive as the

https://www.drupal.org/project/cors (Drupal 7)

https://github.com/piyuesh23/cors (Drupal 8) I added a fix for D8 Beta 10

Someone should contact piyuesh23 about moving it over.

keboca’s picture

@decibel.places thanks it is useful, also @pkosenko thanks for a heads-up. I was getting the same error reported
https://github.com/piyuesh23/cors/issues/4
However your patch is good enough, I am working with Drupal 8.0.0-beta14 then I updated implementation of "CorsConfigForm":
Added the following lines:

/**
   * {@inheritdoc}
   */
  protected function getEditableConfigNames() {
    return [
      'cors.config'
    ];
  }

And overwrite the way to retrieve configuration:
$config = $this->configFactory->getEditable('cors.config');

constrict0r’s picture

Version: 8.1.x-dev » 8.0.0-beta16

0n Drupal-8.0.0 beta 16 I applied the patch made by @pkosenko and it worked perfectly.

dawehner’s picture

Version: 8.0.0-beta16 » 8.1.x-dev

Well, this won't land in 8.1.x, well at lest the next version has a chance for that.

clemens.tolboom’s picture

@pkosenko + @keboca + @constrict0r as we have a patch in #63 it would be better to reroll the patch instead of pointing to remote sites. Regarding contrib cors see #2328985: Drupal 8 port for CORS

andypost’s picture

+++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -51,6 +52,8 @@ public function register(ContainerBuilder $container) {
+    $container->addCompilerPass(new CorsCompilerPass());

I'd better make it into module to easy override from contrib and make configurable

jeqq’s picture

This is a reroll, doesn't contain changes from #80

dawehner’s picture

Status: Postponed » Needs review

Let's ask the testbot about it!

Status: Needs review » Needs work

The last submitted patch, 81: enable_cors-1869548-81.patch, failed testing.

dawehner’s picture

We should add the additional library in its issue and integrate it in here.
This way we can iterate quicker.

jeqq’s picture

Status: Needs work » Needs review
FileSize
36.24 KB

Fixed patch #81

Status: Needs review » Needs work

The last submitted patch, 85: enable_cors-1869548-83.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.24 KB

Let's try it again.
Note: We are using a kernel test now.

Status: Needs review » Needs work

The last submitted patch, 87: 1869548-87.patch, failed testing.

AlxVallejo’s picture

Any reason why we need the installed.json file in the patch? That's where it's failing. According to some sources, this file shouldn't be committed to the repository since it's essentially a documentation of what packages are installed on your instance.

AlxVallejo’s picture

Here's the same patch without the installed.json.

One note is that if I set exposedHeaders to true, I get a 403 on just about every request.

AlxVallejo’s picture

Assigned: Unassigned » AlxVallejo
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 90: 1869548-90.patch, failed testing.

AlxVallejo’s picture

Assigned: AlxVallejo » Unassigned
dawehner’s picture

Status: Needs work » Needs review
FileSize
38.24 KB

I'll try it as well.

Status: Needs review » Needs work

The last submitted patch, 94: 1869548-94.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.24 KB

Let's try a binary one.

Status: Needs review » Needs work

The last submitted patch, 96: 1869548-96.patch, failed testing.

dawehner’s picture

No idea why this doesn't apply to be honest.

Wim Leers’s picture

Is this not blocked on #2237231: Support OPTIONS request ?

dawehner’s picture

Afaik OPTIONS requests are not always requested, just in one of many cases, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Needs work » Needs review
FileSize
37.13 KB

Reroll ...

Status: Needs review » Needs work

The last submitted patch, 102: 1869548-102.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.35 KB
2.7 KB

There we go

Wim Leers’s picture

Oh, hah! Nice fix :)

I think somebody else with deeper CORS knowledge/experience should RTBC this. But the patch looks great to me. Only very minor remarks.

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CorsCompilerPass.php
    @@ -0,0 +1,31 @@
    + * @see services.yml
    

    core.services.yml ?

  2. +++ b/sites/default/default.services.yml
    @@ -153,3 +153,19 @@ parameters:
    +    # Specify allowed request methods, specify '*' to allow all possible ones.
    +    allowedMethods: []
    

    This one is a bit confusing. The default value is an array, but we must specify a sole string to allow all methods? Or is it ['*'] that you should specify here?

  3. +++ b/sites/default/default.services.yml
    @@ -153,3 +153,19 @@ parameters:
    +    exposedHeaders: false
    +    maxAge: false
    +    supportsCredentials: false
    

    We have documentation for the other settings, shouldn't we also have them for these?

dawehner’s picture

Here are some minor adaptions ...

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

On second thought, in #105 I said:

I think somebody else with deeper CORS knowledge/experience should RTBC this. But the patch looks great to me. Only very minor remarks.

… but this patch doesn't actually enable CORS by default. This is just to make it possible to enable CORS. That's a big step forward. So let's do this.

This will still need a change record. And ideally, it would be documented. I think https://www.drupal.org/documentation/modules/rest/start would be a good place.

The last submitted patch, 65: 1869548-63-without-vendor-do-not-test.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 106: 1869548-106.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.93 KB

Updated

Wim Leers’s picture

Title: Enable CORS for GET requests » Opt-in CORS support
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

I started a CR myself: https://www.drupal.org/node/2715637

That also made me realize the current issue title is fairly misleading

catch’s picture

Component: rest.module » base system
Status: Reviewed & tested by the community » Needs review

I had a quick read through https://github.com/asm89/stack-cors/blob/master/src/Asm89/Stack/CorsServ...


    private function checkOrigin(Request $request) {
        if ($this->options['allowedOrigins'] === true) {
            // allow all '*' flag
            return true;
        }
        $origin = $request->headers->get('Origin');
        return in_array($origin, $this->options['allowedOrigins']);
    }
    private function checkMethod(Request $request) {
        if ($this->options['allowedMethods'] === true) {
            // allow all '*' flag
            return true;
        }
        $requestMethod = strtoupper($request->headers->get('Access-Control-Request-Method'));
        return in_array($requestMethod, $this->options['allowedMethods']);

The non-strict in_array() comparisons don't look great, especially given the configuration apparently allows an array of methods/origins as well as boolean true for both - I can see a badly resolved merge conflict or similar leading to someone having 'true' in their options array.

+++ b/core/core.services.yml
@@ -700,6 +708,11 @@ services:
   psr7.http_foundation_factory:

Not sure about adding this to core.services.yml without anything using it. For example why wouldn't REST module add it itself if that's where we're going to use it in core?

If we take the configuration from services.yml, if multiple modules want to use CORS (say one for REST and one for AJAX), how do the different configuration needs between the two work? Or is it entirely up to the site administrator to configure this correctly, in which case we're back to the original question of why put it in core.services.yml and undoing it with a compiler pass vs. just letting them configure in a project/site-specific YAML.

Just in general, having this in a middleware doesn't seem like the most useful place. It allows requests to be rejected very early if they're invalid, but that's by far the least common case - so we'll be running this code on requests that don't care about this at all.

Also moving this out of rest module to base system for now.

Wim Leers’s picture

For example why wouldn't REST module add it itself if that's where we're going to use it in core?

Because it lives on a different level than REST. REST = routes + controllers. CORS is something you may want to apply to things other than REST too. CORS is something more fundamental than REST.

If we take the configuration from services.yml, if multiple modules want to use CORS (say one for REST and one for AJAX), how do the different configuration needs between the two work?

That is the thing: it needs to be configured. That's why it's opt-in. We can't automate this, because it has security consequences. So we need developers to configure it to suit their needs.

in which case we're back to the original question of why put it in core.services.yml and undoing it with a compiler pass vs. just letting them configure in a project/site-specific YAML.

To my understanding, core.services.yml contains all of the default configuration, site-specific YAML files can then override container parameters, i.e. any container parameter a site-specific YAML file specifies is one they override.

This is why sites/default/default.services.yml duplicates a lot of container parameters that live in core.services.yml, but it adds documentation. And it's why Drupal's default installation doesn't actually copy sites/default/default.services.yml to sites/default/services.yml. It's an opt-in process. It's exactly the same here.

Just in general, having this in a middleware doesn't seem like the most useful place. It allows requests to be rejected very early if they're invalid, but that's by far the least common case - so we'll be running this code on requests that don't care about this at all.

Having this in a middleware may add CORS headers to some responses unnecessarily, but it does make reasoning about it far simpler: it means that we'll be setting the same CORS headers on all responses. Which indeed may be overkill/unnecessary in some cases, but the lack of conditionality, and the declarativeness we get instead make it super simple to reason about: my site controls domains X, Y and Z, and so they're safe to interact with, so I'll whitelist them. That's simple.
If you instead have to whitelist on a per-URL, per-route or per REST-resource basis, that makes the configuration a whole lot more complex, with no apparent gain. Because if you trust a certain domain

Also, quoting #16:

When adding new functionality to the web platform, it can be tempting to grant a privilege to one resource but to withhold that privilege from another resource in the same origin. However, withholding privileges in this way is ineffective because the resource without the privilege can usually obtain the privilege anyway because user agents do not isolate resources within an origin. Instead, privileges should be granted or withheld from origins as a whole (rather than discriminating between individual resources within an origin) [BOFGO].

dawehner’s picture

If you instead have to whitelist on a per-URL, per-route or per REST-resource basis, that makes the configuration a whole lot more complex, with no apparent gain. Because if you trust a certain domain

So what we do on normal requests is the following:

if ( ! $this->cors->isCorsRequest($request)) {
  return $this->app->handle($request, $type, $catch);
}

and

 public function isCorsRequest(Request $request)
    {
        return $request->headers->has('Origin');
    }

so for normal HTML requests we don't use it anyway. Its hard to see how more minimal it could be.

dawehner’s picture

In general I don't want to fight for that external library, I just think that actual configuration via CMI doesn't give us that much.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Per #114, #115 and #116, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 110: 1869548-110.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Retesting.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Discussed in irc with dawehner.

I have a general concern that middlewares are becoming the new hook_boot(). The usage here is OK, but I have a list as long as my arm of horrible things done in hook_boot() in 7.x and earlier, and the more middlewares we get in core, the more likely it seems we'll get them used as an antipattern in contrib and custom code. Is there high level documentation of them on d.o? If not a follow-up to add some would be good.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CorsCompilerPass.php
@@ -0,0 +1,31 @@
+}

Could this not be reversed so it adds the middleware if it's enabled, rather than removing it if it's disabled? dawehner also brought this up in irc.

Wim Leers’s picture

but I have a list as long as my arm of horrible things done in hook_boot() in 7.x and earlier

:D

and the more middlewares we get in core, the more likely it seems we'll get them used as an antipattern in contrib and custom code. Is there high level documentation of them on d.o? If not a follow-up to add some would be good.

Effectively they indeed are the new hook_boot(). Is this really an anti-pattern? Sometimes there are things you need to do at that stage. What really matters, is that they're as fast as possible, and rely on as few and as cheap services as possible. The devel module's middleware for example is awful, because it starts the entire configuration service… which OTOH seems totally acceptable for that particular module, since it shouldn't ever run on production anyway.
So, my question: what do you want to see happen? Documentation explaining the purpose of middlewares, the caveats, what to pay attention to?

Could this not be reversed so it adds the middleware if it's enabled, rather than removing it if it's disabled? dawehner also brought this up in irc.

That sounds very sensible.

catch’s picture

Effectively they indeed are the new hook_boot(). Is this really an anti-pattern? Sometimes there are things you need to do at that stage. What really matters, is that they're as fast as possible, and rely on as few and as cheap services as possible.

Yes it's this last bit that's important. I've seen hook_boot() calling drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL)

So, my question: what do you want to see happen? Documentation explaining the purpose of middlewares, the caveats, what to pay attention to?

Yes something like this. Also the distinction from kernel request events (which I also don't like for a lot of the things they get used for - those are the new hook_init()).

Wim Leers’s picture

Issue tags: +Needs Documentation

Do you have thoughts on where this documentation should live? I don't see any child page of https://www.drupal.org/developing/api/8 that is relevant (yes, because our documentation is saddeningly incomplete).

andypost’s picture

Looks #2303673: Implement stackphp; cleanup handlePageCache() and preHandle() missed to add change record and docs
Meta is closed also #2492955: [meta] PSR-7 support

As I see there's only https://www.drupal.org/node/2409067 about middleware

catch’s picture

#123 no I had a feeling there was none at all, that's what this issue reminded me of.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Alright, I'll take on working on that documentation and on the reversing mentioned in #120. Then this can go back to RTBC. This is so tantalizingly close.

@dawehner, if you want to work on the reversing, feel free. I'll take on the documentation in any case.

dawehner’s picture

Could this not be reversed so it adds the middleware if it's enabled, rather than removing it if it's disabled? dawehner also brought this up in irc.

I was about to just do it, but then I realized something

a) For better discoverability (use for example ag "- { name: http_middleware }" its worth to have the service inside core.services.yml IMHO
b) I thought about dropping

     tags:
       - { name: http_middleware }

by default, but still the discoverability.
c) The service pass itself used a enabled flag, rather than a disabled flag, so the default behaviour is to not add it to the container, even if the config is dropped entirely.

Given that on runtime we skip the middleware by default, I think the additional discoverability is a good point for keeping it as it is now.

Feel free to disagree.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs Documentation

#127:

a) Agreed.
b) Indeed, that'd result in a partial/incomplete service definition being declarative in a YAML file, and then some logic to make it "complete" in PHP. That's confusing.
c) Right:

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CorsCompilerPass.php
@@ -0,0 +1,31 @@
+    // Remove the CORS middleware completly in case it was not enabled.
+    if (!$enabled) {
+      $container->removeDefinition('http_middleware.cors');
+    }

@catch referred to this exact code in #120 too:

Could this not be reversed so it adds the middleware if it's enabled, rather than removing it if it's disabled? dawehner also brought this up in irc.

So, catch has a general concern about adding middlewares, and them becoming the new hook_boot(). This patch won't actually make that worse, if anything, it makes it better, because the middleware will only effectively be present in the container if cors.config.enabled === true. But that's easily overlooked, is what catch's concern is. To properly explain this, I think we need documentation anyway. Wrote that just now: https://www.drupal.org/developing/api/8/middleware.

That still leaves a matter of preference. Either:

  • declarative service definition in core.services.yml and PHP code to remove it — pro: better discoverability, con: feels a bit strange logically (always add, conditionally remove)
  • PHP code to set the service definition — pro: nicer logically (conditionally added), con: worse discoverability

Given that in Drupal we tend to favor discoverability/DX over logical/semantical purity, I think @dawehner is right. So, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 110: 1869548-110.patch, failed testing.

chr.fritsch’s picture

Rerolled

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 130: opt_in_cors_support-1869548-130.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Failed in unrelated Config.Drupal\config\Tests\ConfigImportAllTest. Retesting, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 130: opt_in_cors_support-1869548-130.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Now a random fail in SqlContentEntityStorageSchemaIndexFilledTest . Sigh.

  • catch committed f2b7115 on 8.2.x
    Issue #1869548 by dawehner, jeqq, dmouse, damiankloip, nod_, AlxVallejo...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Wim Leers’s picture

xjm’s picture

Issue tags: +8.2.0 release notes
alexpott’s picture

Status: Fixed » Needs work
+++ b/composer.lock
@@ -8,6 +8,49 @@
@@ -1061,7 +1104,7 @@

@@ -1061,7 +1104,7 @@
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/php-fig/log/zipball/1.0.0",
+                "url": "https://api.github.com/repos/php-fig/log/zipball/fe0936ee26643249e916849d48e3a51d5f5e278b",
                 "reference": "1.0.0",
                 "shasum": ""
             },
@@ -2716,24 +2759,24 @@

@@ -2716,24 +2759,24 @@
     "packages-dev": [
         {
             "name": "behat/mink",
-            "version": "v1.7.1",
+            "version": "v1.7.0",
             "source": {
                 "type": "git",
                 "url": "https://github.com/minkphp/Mink.git",
-                "reference": "e6930b9c74693dff7f4e58577e1b1743399f3ff9"
+                "reference": "6c129030ec2cc029905cf969a56ca8f087b2dfdf"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/minkphp/Mink/zipball/e6930b9c74693dff7f4e58577e1b1743399f3ff9",
-                "reference": "e6930b9c74693dff7f4e58577e1b1743399f3ff9",
+                "url": "https://api.github.com/repos/minkphp/Mink/zipball/6c129030ec2cc029905cf969a56ca8f087b2dfdf",
+                "reference": "6c129030ec2cc029905cf969a56ca8f087b2dfdf",
                 "shasum": ""
             },
             "require": {
                 "php": ">=5.3.1",
-                "symfony/css-selector": "~2.1|~3.0"
+                "symfony/css-selector": "~2.1"
             },
             "require-dev": {
-                "symfony/phpunit-bridge": "~2.7|~3.0"
+                "symfony/phpunit-bridge": "~2.7"
             },
             "suggest": {
                 "behat/mink-browserkit-driver": "extremely fast headless driver for Symfony\\Kernel-based apps (Sf2, Silex)",
@@ -2770,31 +2813,31 @@

@@ -2770,31 +2813,31 @@
                 "testing",
                 "web"
             ],
-            "time": "2016-03-05 08:26:18"
+            "time": "2015-09-20 20:24:03"
         },
         {
             "name": "behat/mink-browserkit-driver",
-            "version": "v1.3.2",
+            "version": "v1.3.0",
             "source": {
                 "type": "git",
                 "url": "https://github.com/minkphp/MinkBrowserKitDriver.git",
-                "reference": "10e67fb4a295efcd62ea0bf16025a85ea19534fb"
+                "reference": "da47df1593dac132f04d24e7277ef40d33d9f201"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/minkphp/MinkBrowserKitDriver/zipball/10e67fb4a295efcd62ea0bf16025a85ea19534fb",
-                "reference": "10e67fb4a295efcd62ea0bf16025a85ea19534fb",
+                "url": "https://api.github.com/repos/minkphp/MinkBrowserKitDriver/zipball/da47df1593dac132f04d24e7277ef40d33d9f201",
+                "reference": "da47df1593dac132f04d24e7277ef40d33d9f201",
                 "shasum": ""
             },
             "require": {
-                "behat/mink": "^1.7.1@dev",
+                "behat/mink": "~1.7@dev",
                 "php": ">=5.3.6",
-                "symfony/browser-kit": "~2.3|~3.0",
-                "symfony/dom-crawler": "~2.3|~3.0"
+                "symfony/browser-kit": "~2.3",
+                "symfony/dom-crawler": "~2.3"
             },
             "require-dev": {
                 "silex/silex": "~1.2",
-                "symfony/phpunit-bridge": "~2.7|~3.0"
+                "symfony/phpunit-bridge": "~2.7"
             },
             "type": "mink-driver",
             "extra": {
@@ -2826,20 +2869,20 @@

@@ -2826,20 +2869,20 @@
                 "browser",
                 "testing"
             ],
-            "time": "2016-03-05 08:59:47"
+            "time": "2015-09-21 20:56:13"
         },
         {
             "name": "behat/mink-goutte-driver",
-            "version": "v1.2.1",
+            "version": "v1.2.0",
             "source": {
                 "type": "git",
                 "url": "https://github.com/minkphp/MinkGoutteDriver.git",
-                "reference": "8b9ad6d2d95bc70b840d15323365f52fcdaea6ca"
+                "reference": "c8e254f127d6f2242b994afd4339fb62d471df3f"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/minkphp/MinkGoutteDriver/zipball/8b9ad6d2d95bc70b840d15323365f52fcdaea6ca",
-                "reference": "8b9ad6d2d95bc70b840d15323365f52fcdaea6ca",
+                "url": "https://api.github.com/repos/minkphp/MinkGoutteDriver/zipball/c8e254f127d6f2242b994afd4339fb62d471df3f",
+                "reference": "c8e254f127d6f2242b994afd4339fb62d471df3f",
                 "shasum": ""
             },
             "require": {
@@ -2849,7 +2892,7 @@

@@ -2849,7 +2892,7 @@
                 "php": ">=5.3.1"
             },
             "require-dev": {
-                "symfony/phpunit-bridge": "~2.7|~3.0"
+                "symfony/phpunit-bridge": "~2.7"
             },
             "type": "mink-driver",
             "extra": {
@@ -2881,7 +2924,7 @@

@@ -2881,7 +2924,7 @@
                 "headless",
                 "testing"
             ],
-            "time": "2016-03-05 09:04:22"
+            "time": "2015-09-21 21:31:11"
         },
         {
             "name": "doctrine/instantiator",
@@ -2939,24 +2982,24 @@

@@ -2939,24 +2982,24 @@
         },
         {
             "name": "fabpot/goutte",
-            "version": "v3.1.2",
+            "version": "v3.1.1",
             "source": {
                 "type": "git",
                 "url": "https://github.com/FriendsOfPHP/Goutte.git",
-                "reference": "3cbc6ed222422a28400e470050f14928a153207e"
+                "reference": "751a3dc5c4d86ec3e97c9f27133ef9694d9243cc"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/FriendsOfPHP/Goutte/zipball/3cbc6ed222422a28400e470050f14928a153207e",
-                "reference": "3cbc6ed222422a28400e470050f14928a153207e",
+                "url": "https://api.github.com/repos/FriendsOfPHP/Goutte/zipball/751a3dc5c4d86ec3e97c9f27133ef9694d9243cc",
+                "reference": "751a3dc5c4d86ec3e97c9f27133ef9694d9243cc",
                 "shasum": ""
             },
             "require": {
                 "guzzlehttp/guzzle": "^6.0",
                 "php": ">=5.5.0",
-                "symfony/browser-kit": "~2.1|~3.0",
-                "symfony/css-selector": "~2.1|~3.0",
-                "symfony/dom-crawler": "~2.1|~3.0"
+                "symfony/browser-kit": "~2.1",
+                "symfony/css-selector": "~2.1",
+                "symfony/dom-crawler": "~2.1"
             },
             "type": "application",
             "extra": {
@@ -2984,7 +3027,7 @@

@@ -2984,7 +3027,7 @@
             "keywords": [
                 "scraper"
             ],
-            "time": "2015-11-05 12:58:44"
+            "time": "2015-08-29 16:16:56"
         },

This composer change is not right - it's a downgrade for no reason. I'm reverting this patch because we shouldn't be doing this here.

  • alexpott committed e868cdc on 8.2.x
    Revert "Issue #1869548 by dawehner, jeqq, dmouse, damiankloip, nod_,...
alexpott’s picture

Status: Needs work » Needs review
FileSize
6.85 KB
9.22 KB

Here's the fixed composer patch.

The best method to add a new dependency to core/composer.json is a bit obscure.

  1. Add the dependency to core/composer.json
  2. Run composer update NOTHING

Doing the weird second step cause composer to sort out the merge and do an install.

alexpott’s picture

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

My fixes to this patch have little to do the actual functionality - that has been rtbc before :) so... rtbc'ing the issue.

jibran’s picture

+++ b/core/composer.json
@@ -31,7 +31,8 @@
+        "asm89/stack-cors": "~0.2.1"

This version doesn't support symfony 3.0 also see https://github.com/asm89/stack-cors/issues/26. This will hinder our performance in #2712647: Update Symfony components to ~3.2. We should consider changing it to dev-master here like #2776105: Update asm89/stack-cors for Symfony 3 compatibility and revert changes introduced by #1869548.

dawehner’s picture

@jibran
But we have a way to solve that ... IMHO updating to symfony 3 is not a requirement for 8.2. 2.8 will be supported for longer.

alexpott’s picture

I think we should ask for an official release - see https://github.com/asm89/stack-cors/issues/26 and get that before re-committing. Symfony 3 is a known and big target. Committing dev-master dependencies is not a good idea.

alexpott’s picture

But using a reference is better than just a value copy because exception traces are not always something small.

slasher13’s picture

Assigned: Unassigned » slasher13
slasher13’s picture

Assigned: slasher13 » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
9.92 KB
2.69 KB
alexpott’s picture

Status: Needs review » Needs work

So #150 bumped the version to 1.0.0 - nice.

+++ b/composer.lock
@@ -1100,12 +1100,12 @@
-                "reference": "1.0.0"
+                "reference": "fe0936ee26643249e916849d48e3a51d5f5e278b"
...
+                "url": "https://api.github.com/repos/php-fig/log/zipball/fe0936ee26643249e916849d48e3a51d5f5e278b",
+                "reference": "fe0936ee26643249e916849d48e3a51d5f5e278b",
                 "shasum": ""

These are completely unrelated changes - not sure why they occur but they don't for me.

slasher13’s picture

Status: Needs work » Needs review
FileSize
9.23 KB
828 bytes

Changes occur after composer self-update. Can you confirm it?

alexpott’s picture

@slasher13 not for me. My composer version is Composer version 1.2.0 2016-07-19 01:28:52. The procedure I follow is outlined in #142.

slasher13’s picture

I could reproduce my problem.

  • If i run initially composer install with an empty vendor folder and then composer update NOTHING, no package information changes.
  • If i run initially composer update NOTHING with an empty vendor folder and then composer update NOTHING again, package information changes.
alexpott’s picture

@slasher13 nice sleuthing. It's one of the tricky things about composer.lock diffs - people have got too used to stuff changing for no reason. I think it's best for changes to be the minimal change - which #152 has. Nice.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It seems to be that @alexpott and @slasher13 agree on #152, so this is ready again, nice!

Thank you @slasher13 for jumping in!

xjm’s picture

Issue tags: +beta deadline

Yay!

This most likely needs to go in before the beta commit freeze (tomorrow) to be in 8.2.x, so tagging for visibility in the RTBC queue.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed bc83416 and pushed to 8.2.x. Thanks!

  • alexpott committed bc83416 on 8.2.x
    Issue #1869548 by dawehner, jeqq, dmouse, slasher13, damiankloip,...

  • catch committed f2b7115 on 8.3.x
    Issue #1869548 by dawehner, jeqq, dmouse, damiankloip, nod_, AlxVallejo...
  • alexpott committed bc83416 on 8.3.x
    Issue #1869548 by dawehner, jeqq, dmouse, slasher13, damiankloip,...
  • alexpott committed e868cdc on 8.3.x
    Revert "Issue #1869548 by dawehner, jeqq, dmouse, damiankloip, nod_,...

  • catch committed f2b7115 on 8.3.x
    Issue #1869548 by dawehner, jeqq, dmouse, damiankloip, nod_, AlxVallejo...
  • alexpott committed bc83416 on 8.3.x
    Issue #1869548 by dawehner, jeqq, dmouse, slasher13, damiankloip,...
  • alexpott committed e868cdc on 8.3.x
    Revert "Issue #1869548 by dawehner, jeqq, dmouse, damiankloip, nod_,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Sam152’s picture

Just noting this was really really hard to override from a config object. See #2794109: Find a way to replace the container parameter in a better way..

Re: andypost, #80, looks like we had the same thought :)