{"action":"closed","issue":{"url":"https://api.github.com/repos/zulip/zulip/issues/8779","repository_url":"https://api.github.com/repos/zulip/zulip","labels_url":"https://api.github.com/repos/zulip/zulip/issues/8779/labels{/name}","comments_url":"https://api.github.com/repos/zulip/zulip/issues/8779/comments","events_url":"https://api.github.com/repos/zulip/zulip/issues/8779/events","html_url":"https://github.com/zulip/zulip/issues/8779","id":307612264,"number":8779,"title":"parse_user_agent matches all strings","user":{"login":"jckw","id":11884004,"avatar_url":"https://avatars1.githubusercontent.com/u/11884004?v=4","gravatar_id":"","url":"https://api.github.com/users/jckw","html_url":"https://github.com/jckw","followers_url":"https://api.github.com/users/jckw/followers","following_url":"https://api.github.com/users/jckw/following{/other_user}","gists_url":"https://api.github.com/users/jckw/gists{/gist_id}","starred_url":"https://api.github.com/users/jckw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jckw/subscriptions","organizations_url":"https://api.github.com/users/jckw/orgs","repos_url":"https://api.github.com/users/jckw/repos","events_url":"https://api.github.com/users/jckw/events{/privacy}","received_events_url":"https://api.github.com/users/jckw/received_events","type":"User","site_admin":false},"labels":[{"id":545030708,"url":"https://api.github.com/repos/zulip/zulip/labels/in%20progress","name":"in progress","color":"bfdadc","default":false}],"state":"closed","locked":false,"assignee":{"login":"jckw","id":11884004,"avatar_url":"https://avatars1.githubusercontent.com/u/11884004?v=4","gravatar_id":"","url":"https://api.github.com/users/jckw","html_url":"https://github.com/jckw","followers_url":"https://api.github.com/users/jckw/followers","following_url":"https://api.github.com/users/jckw/following{/other_user}","gists_url":"https://api.github.com/users/jckw/gists{/gist_id}","starred_url":"https://api.github.com/users/jckw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jckw/subscriptions","organizations_url":"https://api.github.com/users/jckw/orgs","repos_url":"https://api.github.com/users/jckw/repos","events_url":"https://api.github.com/users/jckw/events{/privacy}","received_events_url":"https://api.github.com/users/jckw/received_events","type":"User","site_admin":false},"assignees":[{"login":"jckw","id":11884004,"avatar_url":"https://avatars1.githubusercontent.com/u/11884004?v=4","gravatar_id":"","url":"https://api.github.com/users/jckw","html_url":"https://github.com/jckw","followers_url":"https://api.github.com/users/jckw/followers","following_url":"https://api.github.com/users/jckw/following{/other_user}","gists_url":"https://api.github.com/users/jckw/gists{/gist_id}","starred_url":"https://api.github.com/users/jckw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jckw/subscriptions","organizations_url":"https://api.github.com/users/jckw/orgs","repos_url":"https://api.github.com/users/jckw/repos","events_url":"https://api.github.com/users/jckw/events{/privacy}","received_events_url":"https://api.github.com/users/jckw/received_events","type":"User","site_admin":false}],"milestone":null,"comments":4,"created_at":"2018-03-22T11:54:30Z","updated_at":"2018-03-22T21:30:20Z","closed_at":"2018-03-22T21:30:20Z","author_association":"COLLABORATOR","body":"The regex `^(?P<name>[^/ ]*[^0-9/(]*)(/(?P<version>[^/ ]*))?([ /].*)?$` matches the empty string with `name`; and since version is optional (but can include linebreaks) and the last group catches everything but linebreaks, there is always a match at [lib/user_agent.py line 8](https://github.com/zulip/zulip/blob/master/zerver/lib/user_agent.py#L8). \r\n\r\nThis means that `match` is never `None` so:\r\n\r\n* Test coverage can't be 100% for lib/user_agent.py\r\n* The [TestUserAgentParsing test](https://github.com/zulip/zulip/blob/master/zerver/tests/test_decorators.py#L1327) is futile (since it will trivially pass as `parse_user_agent` is never `None`)\r\n* Other places where some `user_agent is None` test is carried out (e.g. [1](https://github.com/zulip/zulip/blob/master/zerver/views/compatibility.py#L10), [2](https://github.com/zulip/zulip/blob/master/zerver/decorator.py#L146)), the `else` clause cannot be reached and therefore coverage can never be 100%.\r\n\r\nAlso, nowhere currently uses `user_agent['version']`.\r\n\r\nI think the only option for fixing this bug it to use stricter RegEx (making the version required, or preventing `name` matching with the empty string). Changing the guard conditions from `match is None` can always be done by changing the RegEx pattern, which is cleaner. \r\n\r\nI'll happily update this, just need some advice on what we actually want to match!"}}
{"actor":{"display_login":"timabbott"}}