Skip to content

fix accept header handling when client sends no accept header #134

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.

Sign up for GitHub

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

Merged
ddeboer merged 1 commit into from
Nov 21, 2014
Merged

fix accept header handling when client sends no accept header #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Changelog
=========

1.1.2
-----

* **2014-11-17** Fixed documentation for user context varnish configuration to also work when
client omits the `Accept` HTTP header.
2 changes: 2 additions & 0 deletions tests/.htaccess
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
deny from all

22 changes: 14 additions & 8 deletions tests/Functional/Fixtures/varnish-3/user_context.vcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ sub vcl_recv {
&& (req.http.cookie || req.http.authorization)
&& (req.request == "GET" || req.request == "HEAD")
) {
set req.http.x-fos-original-url = req.url;
set req.http.x-fos-original-accept = req.http.accept;

set req.http.accept = "application/vnd.fos.user-context-hash";
set req.http.x-fos-original-url = req.url;
# Backup accept header, if set
if (req.http.accept) {
set req.http.x-fos-original-accept = req.http.accept;
}
set req.http.accept = "application/vnd.fos.user-context-hash";

# A little hack for testing all scenarios. Choose one for your application.
if ("failure" == req.http.x-cache-hash) {
Expand All @@ -38,11 +40,15 @@ sub vcl_recv {
if (req.restarts > 0
&& req.http.accept == "application/vnd.fos.user-context-hash"
) {
set req.url = req.http.x-fos-original-url;
set req.http.accept = req.http.x-fos-original-accept;

set req.url = req.http.x-fos-original-url;
unset req.http.x-fos-original-url;
unset req.http.x-fos-original-accept;
if (req.http.x-fos-original-accept) {
set req.http.accept = req.http.x-fos-original-accept;
unset req.http.x-fos-original-accept;
} else {
# If accept header was not set in original request, remove the header here.
unset req.http.accept;
}

# Force the lookup, the backend must tell not to cache or vary on the
# user hash to properly separate cached data.
Expand Down
22 changes: 14 additions & 8 deletions tests/Functional/Fixtures/varnish-4/user_context.vcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ sub vcl_recv {
&& (req.http.cookie || req.http.authorization)
&& (req.method == "GET" || req.method == "HEAD")
) {
set req.http.x-fos-original-url = req.url;
set req.http.x-fos-original-accept = req.http.accept;

set req.http.accept = "application/vnd.fos.user-context-hash";
set req.http.x-fos-original-url = req.url;
# Backup accept header, if set
if (req.http.accept) {
set req.http.x-fos-original-accept = req.http.accept;
}
set req.http.accept = "application/vnd.fos.user-context-hash";

# A little hack for testing all scenarios. Choose one for your application.
if ("failure" == req.http.x-cache-hash) {
Expand All @@ -40,11 +42,15 @@ sub vcl_recv {
if (req.restarts > 0
&& req.http.accept == "application/vnd.fos.user-context-hash"
) {
set req.url = req.http.x-fos-original-url;
set req.http.accept = req.http.x-fos-original-accept;

set req.url = req.http.x-fos-original-url;
unset req.http.x-fos-original-url;
unset req.http.x-fos-original-accept;
if (req.http.x-fos-original-accept) {
set req.http.accept = req.http.x-fos-original-accept;
unset req.http.x-fos-original-accept;
} else {
# If accept header was not set in original request, remove the header here.
unset req.http.accept;
}

# Force the lookup, the backend must tell not to cache or vary on the
# user hash to properly separate cached data.
Expand Down
10 changes: 10 additions & 0 deletions tests/Functional/Fixtures/web/user_context.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,16 @@

header('X-Cache-Debug: 1');

if (isset($_GET['accept'])) {
if ($_GET['accept'] != $_SERVER['HTTP_ACCEPT']) {
header('HTTP/1.1 500 Wrong accept header "' . $_SERVER['HTTP_ACCEPT'] . '", expected "' . $_GET['accept'] . '"');
Copy link
Contributor Author

@dbu dbu Nov 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for better error reporting. i think it makes sense here but in any system running anywhere it would be dangerous to blindly echo $_SERVER and $_GET values. should we put a .htaccess in tests/ to prevent execution if somebody manages to expose his vendor directory?

Copy link
Member

@ddeboer ddeboer Nov 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we’re doing this in more places, e.g. negotation.php. Could you add a .htaccess that protects the fixtures but still works when running our tests through Apache?

Copy link
Contributor Author

@dbu dbu Nov 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added .htaccess in tests root directory

exit;
}
} elseif (isset($_SERVER['HTTP_ACCEPT'])) {
header('HTTP/1.1 500 Expected no accept header ' . $_SERVER['HTTP_ACCEPT']);
exit;
}

if ('POST' == strtoupper($_SERVER['REQUEST_METHOD'])) {
echo "POST";
exit;
Expand Down
15 changes: 15 additions & 0 deletions tests/Functional/Varnish/UserContextTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ abstract class UserContextTestCase extends VarnishTestCase
*/
abstract protected function assertContextCache($hashCache);

/**
* Sending requests without an Accept: header so none should arrive at the
* backend for the actual request.
*/
public function testUserContextHash()
{
$response1 = $this->getResponse('/user_context.php', array(), array('cookies' => array('foo')));
Expand Down Expand Up @@ -64,6 +68,17 @@ public function testUserContextHash()
$this->assertHit($headResponse2);
}

public function testAcceptHeader()
{
$response1 = $this->getResponse(
'/user_context.php?accept=text/plain',
Copy link
Contributor Author

@dbu dbu Nov 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just a weird hack to not need more backend php files for the testing - the permutations would lead to many files.

array('Accept' => 'text/plain'),
array('cookies' => array('foo'))
);
$this->assertEquals('foo', $response1->getBody(true));

}

public function testUserContextUnauthorized()
{
try {
Expand Down