varnishcache / varnish-devicedetect

VCL based device detection for Varnish Cache.
Other
299 stars 88 forks source link

Tighten up surface requirements to only apply to ARM-based systems #31

Closed KeyserSosa closed 8 years ago

KeyserSosa commented 8 years ago

Alos fixup unit tests to cover more of the current set of common browsers as base cases. The consensus seems to be that only ARM based Windows RT surfaces (2-3 years old now) should really be considered tablets. One corroborating repository for that assessment:

https://github.com/serbanghita/Mobile-Detect/blob/master/Mobile_Detect.php#L227

I'm equally happy to just remove that change as it seems small edge case.

RyanJarv commented 8 years ago

We attempted to use this devicedetect repo but ran into issues with several users getting the mobile version of the site. After a while of searching I found a Windows 8.1 Chrome User-Agent matching the old regular expression here. Below is the User-agent.

Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

I'm not sure why the semi-colon in the regular expression is being ignored but this does in fact match and can be tested on varnish 4.1 with the following varnishtest.

varnishtest "Device detection"

server s1 {
   rxreq
   txresp
} -start

varnish v1 -vcl+backend {
  sub vcl_recv {
    if (req.http.User-Agent ~ "Touch.+Tablet PC" ||
      req.http.User-Agent ~ "Windows NT [0-9.]+; [ARM;|WOW64;|Win64; x64]" ) {
      set req.http.X-UA-Device = "tablet-microsoft";
    } else {
      set req.http.X-UA-Device = "Something-Else";
    }
  }
  sub vcl_deliver {
    set resp.http.X-UA-Device = req.http.X-UA-Device;
  }
} -start

client c1 {
  txreq -hdr "User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36"
  rxresp
  expect resp.http.X-UA-Device != "tablet-microsoft"
} -run

I hope this can get merged in soon. Right now this device detection seems to be broken for 64 bit chrome windows users.

RyanJarv commented 8 years ago

Looking at the original regex again I imagine it was supposed to be this:

if (req.http.User-Agent ~ "Touch.+Tablet PC" ||
      req.http.User-Agent ~ "Windows NT [0-9.]+; (ARM;|WOW64;|Win64; x64)" )

Not the parenthesis instead of square brackets. But either way if this is overly broad then it probably needs to be removed.

fgsch commented 8 years ago

@KeyserSosa I think you should commit this. Whether matching on ARM alone (and not something else as Mobile-Detect does) can be tweaked later. As things stand at the moment this is broken for some users as reported by @RyanJarv .

KeyserSosa commented 8 years ago

Merged