GitHub PullRequestReviewCommentEvent at 2014-09-11 00:37:03
Repository: securedrop
Actor: garrettr
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/freedomofpress/securedrop/pulls/comments/17398349","id":17398349,"diff_hunk":"@@ -277,11 +277,16 @@ def valid_codename(codename):\n def login():\n     if request.method == 'POST':\n         codename = request.form['codename']\n-        if valid_codename(codename):\n-            session.update(codename=codename, logged_in=True)\n-            return redirect(url_for('lookup', from_login='1'))\n+        try:\n+            valid_codename(codename)\n+        except crypto_util.CryptoException:\n+            flash(\"Sorry, that codename contains invalid characters.\", \"error\")\n         else:\n-            flash(\"Sorry, that is not a recognized codename.\", \"error\")\n+            if valid_codename(codename):","path":"securedrop/source.py","position":13,"original_position":13,"commit_id":"e7a9c595456700c6f32016283c1f94a4bd6b2567","original_commit_id":"e7a9c595456700c6f32016283c1f94a4bd6b2567","user":{"login":"garrettr","id":407302,"avatar_url":"https://avatars.githubusercontent.com/u/407302?v=2","gravatar_id":"92b8d7a506b6bc2e97d91a3ea179e56e","url":"https://api.github.com/users/garrettr","html_url":"https://github.com/garrettr","followers_url":"https://api.github.com/users/garrettr/followers","following_url":"https://api.github.com/users/garrettr/following{/other_user}","gists_url":"https://api.github.com/users/garrettr/gists{/gist_id}","starred_url":"https://api.github.com/users/garrettr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/garrettr/subscriptions","organizations_url":"https://api.github.com/users/garrettr/orgs","repos_url":"https://api.github.com/users/garrettr/repos","events_url":"https://api.github.com/users/garrettr/events{/privacy}","received_events_url":"https://api.github.com/users/garrettr/received_events","type":"User","site_admin":false},"body":"It's best to only call `valid_codename` once, both as a coding principle and also because it calls scrypt, which is quite slow (several hundred ms). Can you rewrite this to something like:\r\n\r\n```\r\ntry:\r\n    valid = valid_codename(codename)\r\nexcept crypto_util.CryptoException:\r\n    # flash error\r\nelse:\r\n    if valid:\r\n        # update session and redirect\r\n    else:\r\n        # flash\r\n```","created_at":"2014-09-11T00:37:03Z","updated_at":"2014-09-11T00:37:03Z","html_url":"https://github.com/freedomofpress/securedrop/pull/563#discussion_r17398349","pull_request_url":"https://api.github.com/repos/freedomofpress/securedrop/pulls/563","_links":{"self":{"href":"https://api.github.com/repos/freedomofpress/securedrop/pulls/comments/17398349"},"html":{"href":"https://github.com/freedomofpress/securedrop/pull/563#discussion_r17398349"},"pull_request":{"href":"https://api.github.com/repos/freedomofpress/securedrop/pulls/563"}}},"pull_request":{"url":"https://api.github.com/repos/freedomofpress/securedrop/pulls/563","id":21057004,"html_url":"https://github.com/freedomofpress/securedrop/pull/563","diff_url":"https://github.com/freedomofpress/securedrop/pull/563.diff","patch_url":"https://github.com/freedomofpress/securedrop/pull/563.patch","issue_url":"https://api.github.com/repos/freedomofpress/securedrop/issues/563","number":563,"state":"open","locked":false,"title":"Catch when a users codename contains invalid characters","user":{"login":"DonnchaC","id":3081375,"avatar_url":"https://avatars.githubusercontent.com/u/3081375?v=2","gravatar_id":"e7aad85dd4dabc40edd24a2147f53a35","url":"https://api.github.com/users/DonnchaC","html_url":"https://github.com/DonnchaC","followers_url":"https://api.github.com/users/DonnchaC/followers","following_url":"https://api.github.com/users/DonnchaC/following{/other_user}","gists_url":"https://api.github.com/users/DonnchaC/gists{/gist_id}","starred_url":"https://api.github.com/users/DonnchaC/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DonnchaC/subscriptions","organizations_url":"https://api.github.com/users/DonnchaC/orgs","repos_url":"https://api.github.com/users/DonnchaC/repos","events_url":"https://api.github.com/users/DonnchaC/events{/privacy}","received_events_url":"https://api.github.com/users/DonnchaC/received_events","type":"User","site_admin":false},"body":"fixes #548. Catch the error produced when a user specifies a codename containing potentially dangerous characters, and flash that user an informative message.","created_at":"2014-09-10T23:08:46Z","updated_at":"2014-09-11T00:37:03Z","closed_at":null,"merged_at":null,"merge_commit_sha":"67e023aeb0b3f70b1e16f37195d7970881413b56","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/freedomofpress/securedrop/pulls/563/commits","review_comments_url":"https://api.github.com/repos/freedomofpress/securedrop/pulls/563/comments","review_comment_url":"https://api.github.com/repos/freedomofpress/securedrop/pulls/comments/{number}","comments_url":"https://api.github.com/repos/freedomofpress/securedrop/issues/563/comments","statuses_url":"https://api.github.com/repos/freedomofpress/securedrop/statuses/e7a9c595456700c6f32016283c1f94a4bd6b2567","head":{"label":"DonnchaC:invalid_codename_handling","ref":"invalid_codename_handling","sha":"e7a9c595456700c6f32016283c1f94a4bd6b2567","user":{"login":"DonnchaC","id":3081375,"avatar_url":"https://avatars.githubusercontent.com/u/3081375?v=2","gravatar_id":"e7aad85dd4dabc40edd24a2147f53a35","url":"https://api.github.com/users/DonnchaC","html_url":"https://github.com/DonnchaC","followers_url":"https://api.github.com/users/DonnchaC/followers","following_url":"https://api.github.com/users/DonnchaC/following{/other_user}","gists_url":"https://api.github.com/users/DonnchaC/gists{/gist_id}","starred_url":"https://api.github.com/users/DonnchaC/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DonnchaC/subscriptions","organizations_url":"https://api.github.com/users/DonnchaC/orgs","repos_url":"https://api.github.com/users/DonnchaC/repos","events_url":"https://api.github.com/users/DonnchaC/events{/privacy}","received_events_url":"https://api.github.com/users/DonnchaC/received_events","type":"User","site_admin":false},"repo":{"id":23891285,"name":"securedrop","full_name":"DonnchaC/securedrop","owner":{"login":"DonnchaC","id":3081375,"avatar_url":"https://avatars.githubusercontent.com/u/3081375?v=2","gravatar_id":"e7aad85dd4dabc40edd24a2147f53a35","url":"https://api.github.com/users/DonnchaC","html_url":"https://github.com/DonnchaC","followers_url":"https://api.github.com/users/DonnchaC/followers","following_url":"https://api.github.com/users/DonnchaC/following{/other_user}","gists_url":"https://api.github.com/users/DonnchaC/gists{/gist_id}","starred_url":"https://api.github.com/users/DonnchaC/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DonnchaC/subscriptions","organizations_url":"https://api.github.com/users/DonnchaC/orgs","repos_url":"https://api.github.com/users/DonnchaC/repos","events_url":"https://api.github.com/users/DonnchaC/events{/privacy}","received_events_url":"https://api.github.com/users/DonnchaC/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/DonnchaC/securedrop","description":"GitHub repository for the SecureDrop whistleblower platform. Do not submit documents here!","fork":true,"url":"https://api.github.com/repos/DonnchaC/securedrop","forks_url":"https://api.github.com/repos/DonnchaC/securedrop/forks","keys_url":"https://api.github.com/repos/DonnchaC/securedrop/keys{/key_id}","collaborators_url":"https://api.github.com/repos/DonnchaC/securedrop/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/DonnchaC/securedrop/teams","hooks_url":"https://api.github.com/repos/DonnchaC/securedrop/hooks","issue_events_url":"https://api.github.com/repos/DonnchaC/securedrop/issues/events{/number}","events_url":"https://api.github.com/repos/DonnchaC/securedrop/events","assignees_url":"https://api.github.com/repos/DonnchaC/securedrop/assignees{/user}","branches_url":"https://api.github.com/repos/DonnchaC/securedrop/branches{/branch}","tags_url":"https://api.github.com/repos/DonnchaC/securedrop/tags","blobs_url":"https://api.github.com/repos/DonnchaC/securedrop/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/DonnchaC/securedrop/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/DonnchaC/securedrop/git/refs{/sha}","trees_url":"https://api.github.com/repos/DonnchaC/securedrop/git/trees{/sha}","statuses_url":"https://api.github.com/repos/DonnchaC/securedrop/statuses/{sha}","languages_url":"https://api.github.com/repos/DonnchaC/securedrop/languages","stargazers_url":"https://api.github.com/repos/DonnchaC/securedrop/stargazers","contributors_url":"https://api.github.com/repos/DonnchaC/securedrop/contributors","subscribers_url":"https://api.github.com/repos/DonnchaC/securedrop/subscribers","subscription_url":"https://api.github.com/repos/DonnchaC/securedrop/subscription","commits_url":"https://api.github.com/repos/DonnchaC/securedrop/commits{/sha}","git_commits_url":"https://api.github.com/repos/DonnchaC/securedrop/git/commits{/sha}","comments_url":"https://api.github.com/repos/DonnchaC/securedrop/comments{/number}","issue_comment_url":"https://api.github.com/repos/DonnchaC/securedrop/issues/comments/{number}","contents_url":"https://api.github.com/repos/DonnchaC/securedrop/contents/{+path}","compare_url":"https://api.github.com/repos/DonnchaC/securedrop/compare/{base}...{head}","merges_url":"https://api.github.com/repos/DonnchaC/securedrop/merges","archive_url":"https://api.github.com/repos/DonnchaC/securedrop/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/DonnchaC/securedrop/downloads","issues_url":"https://api.github.com/repos/DonnchaC/securedrop/issues{/number}","pulls_url":"https://api.github.com/repos/DonnchaC/securedrop/pulls{/number}","milestones_url":"https://api.github.com/repos/DonnchaC/securedrop/milestones{/number}","notifications_url":"https://api.github.com/repos/DonnchaC/securedrop/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/DonnchaC/securedrop/labels{/name}","releases_url":"https://api.github.com/repos/DonnchaC/securedrop/releases{/id}","created_at":"2014-09-10T20:37:55Z","updated_at":"2014-09-10T18:29:25Z","pushed_at":"2014-09-10T23:43:59Z","git_url":"git://github.com/DonnchaC/securedrop.git","ssh_url":"git@github.com:DonnchaC/securedrop.git","clone_url":"https://github.com/DonnchaC/securedrop.git","svn_url":"https://github.com/DonnchaC/securedrop","homepage":"https://pressfreedomfoundation.org/securedrop","size":24656,"stargazers_count":0,"watchers_count":0,"language":null,"has_issues":false,"has_downloads":true,"has_wiki":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"develop"}},"base":{"label":"freedomofpress:develop","ref":"develop","sha":"c6d114b1a1e47685454a5aef5ea650038edaf6ce","user":{"login":"freedomofpress","id":5388147,"avatar_url":"https://avatars.githubusercontent.com/u/5388147?v=2","gravatar_id":"35e28c7952837f463228a122c941c213","url":"https://api.github.com/users/freedomofpress","html_url":"https://github.com/freedomofpress","followers_url":"https://api.github.com/users/freedomofpress/followers","following_url":"https://api.github.com/users/freedomofpress/following{/other_user}","gists_url":"https://api.github.com/users/freedomofpress/gists{/gist_id}","starred_url":"https://api.github.com/users/freedomofpress/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/freedomofpress/subscriptions","organizations_url":"https://api.github.com/users/freedomofpress/orgs","repos_url":"https://api.github.com/users/freedomofpress/repos","events_url":"https://api.github.com/users/freedomofpress/events{/privacy}","received_events_url":"https://api.github.com/users/freedomofpress/received_events","type":"Organization","site_admin":false},"repo":{"id":3156820,"name":"securedrop","full_name":"freedomofpress/securedrop","owner":{"login":"freedomofpress","id":5388147,"avatar_url":"https://avatars.githubusercontent.com/u/5388147?v=2","gravatar_id":"35e28c7952837f463228a122c941c213","url":"https://api.github.com/users/freedomofpress","html_url":"https://github.com/freedomofpress","followers_url":"https://api.github.com/users/freedomofpress/followers","following_url":"https://api.github.com/users/freedomofpress/following{/other_user}","gists_url":"https://api.github.com/users/freedomofpress/gists{/gist_id}","starred_url":"https://api.github.com/users/freedomofpress/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/freedomofpress/subscriptions","organizations_url":"https://api.github.com/users/freedomofpress/orgs","repos_url":"https://api.github.com/users/freedomofpress/repos","events_url":"https://api.github.com/users/freedomofpress/events{/privacy}","received_events_url":"https://api.github.com/users/freedomofpress/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/freedomofpress/securedrop","description":"GitHub repository for the SecureDrop whistleblower platform. Do not submit documents here!","fork":false,"url":"https://api.github.com/repos/freedomofpress/securedrop","forks_url":"https://api.github.com/repos/freedomofpress/securedrop/forks","keys_url":"https://api.github.com/repos/freedomofpress/securedrop/keys{/key_id}","collaborators_url":"https://api.github.com/repos/freedomofpress/securedrop/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/freedomofpress/securedrop/teams","hooks_url":"https://api.github.com/repos/freedomofpress/securedrop/hooks","issue_events_url":"https://api.github.com/repos/freedomofpress/securedrop/issues/events{/number}","events_url":"https://api.github.com/repos/freedomofpress/securedrop/events","assignees_url":"https://api.github.com/repos/freedomofpress/securedrop/assignees{/user}","branches_url":"https://api.github.com/repos/freedomofpress/securedrop/branches{/branch}","tags_url":"https://api.github.com/repos/freedomofpress/securedrop/tags","blobs_url":"https://api.github.com/repos/freedomofpress/securedrop/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/freedomofpress/securedrop/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/freedomofpress/securedrop/git/refs{/sha}","trees_url":"https://api.github.com/repos/freedomofpress/securedrop/git/trees{/sha}","statuses_url":"https://api.github.com/repos/freedomofpress/securedrop/statuses/{sha}","languages_url":"https://api.github.com/repos/freedomofpress/securedrop/languages","stargazers_url":"https://api.github.com/repos/freedomofpress/securedrop/stargazers","contributors_url":"https://api.github.com/repos/freedomofpress/securedrop/contributors","subscribers_url":"https://api.github.com/repos/freedomofpress/securedrop/subscribers","subscription_url":"https://api.github.com/repos/freedomofpress/securedrop/subscription","commits_url":"https://api.github.com/repos/freedomofpress/securedrop/commits{/sha}","git_commits_url":"https://api.github.com/repos/freedomofpress/securedrop/git/commits{/sha}","comments_url":"https://api.github.com/repos/freedomofpress/securedrop/comments{/number}","issue_comment_url":"https://api.github.com/repos/freedomofpress/securedrop/issues/comments/{number}","contents_url":"https://api.github.com/repos/freedomofpress/securedrop/contents/{+path}","compare_url":"https://api.github.com/repos/freedomofpress/securedrop/compare/{base}...{head}","merges_url":"https://api.github.com/repos/freedomofpress/securedrop/merges","archive_url":"https://api.github.com/repos/freedomofpress/securedrop/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/freedomofpress/securedrop/downloads","issues_url":"https://api.github.com/repos/freedomofpress/securedrop/issues{/number}","pulls_url":"https://api.github.com/repos/freedomofpress/securedrop/pulls{/number}","milestones_url":"https://api.github.com/repos/freedomofpress/securedrop/milestones{/number}","notifications_url":"https://api.github.com/repos/freedomofpress/securedrop/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/freedomofpress/securedrop/labels{/name}","releases_url":"https://api.github.com/repos/freedomofpress/securedrop/releases{/id}","created_at":"2012-01-11T19:57:55Z","updated_at":"2014-09-10T18:29:25Z","pushed_at":"2014-09-09T00:34:29Z","git_url":"git://github.com/freedomofpress/securedrop.git","ssh_url":"git@github.com:freedomofpress/securedrop.git","clone_url":"https://github.com/freedomofpress/securedrop.git","svn_url":"https://github.com/freedomofpress/securedrop","homepage":"https://pressfreedomfoundation.org/securedrop","size":24656,"stargazers_count":1222,"watchers_count":1222,"language":"Shell","has_issues":true,"has_downloads":true,"has_wiki":false,"forks_count":202,"mirror_url":null,"open_issues_count":161,"forks":202,"open_issues":161,"watchers":1222,"default_branch":"develop"}},"_links":{"self":{"href":"https://api.github.com/repos/freedomofpress/securedrop/pulls/563"},"html":{"href":"https://github.com/freedomofpress/securedrop/pull/563"},"issue":{"href":"https://api.github.com/repos/freedomofpress/securedrop/issues/563"},"comments":{"href":"https://api.github.com/repos/freedomofpress/securedrop/issues/563/comments"},"review_comments":{"href":"https://api.github.com/repos/freedomofpress/securedrop/pulls/563/comments"},"review_comment":{"href":"https://api.github.com/repos/freedomofpress/securedrop/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/freedomofpress/securedrop/pulls/563/commits"},"statuses":{"href":"https://api.github.com/repos/freedomofpress/securedrop/statuses/e7a9c595456700c6f32016283c1f94a4bd6b2567"}}}}