{"action":"created","comment":{"url":"https://api.github.com/repos/stellar/horizon/pulls/comments/101418588","pull_request_review_id":22146639,"id":101418588,"diff_hunk":"@@ -71,3 +72,16 @@ func (this *Account) Populate(\n \tthis.Links.Data.PopulateTemplated()\n \treturn\n }\n+\n+// GetData returns decoded value for a given key. If the key does\n+// not exist, empty slice will be returned.\n+func (this *Account) GetData(key string) []byte {\n+\tdata, exists := this.Data[key]\n+\tif !exists {\n+\t\treturn nil\n+\t}\n+\n+\t// We assume that horizon always returns valid base64 encoded string\n+\tdecoded, _ := base64.StdEncoding.DecodeString(data)","path":"src/github.com/stellar/horizon/resource/account.go","position":22,"original_position":22,"commit_id":"04d7fae802557a909ae045eb1c164d06e4f1142a","original_commit_id":"04d7fae802557a909ae045eb1c164d06e4f1142a","user":{"login":"nullstyle","id":366,"avatar_url":"https://avatars.githubusercontent.com/u/366?v=3","gravatar_id":"","url":"https://api.github.com/users/nullstyle","html_url":"https://github.com/nullstyle","followers_url":"https://api.github.com/users/nullstyle/followers","following_url":"https://api.github.com/users/nullstyle/following{/other_user}","gists_url":"https://api.github.com/users/nullstyle/gists{/gist_id}","starred_url":"https://api.github.com/users/nullstyle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nullstyle/subscriptions","organizations_url":"https://api.github.com/users/nullstyle/orgs","repos_url":"https://api.github.com/users/nullstyle/repos","events_url":"https://api.github.com/users/nullstyle/events{/privacy}","received_events_url":"https://api.github.com/users/nullstyle/received_events","type":"User","site_admin":false},"body":"> If many of the things I listed are validated by horizon, then why can't we validate data value too (valid base64) and allow developers to decode it with assumption that it's correct? (so GetData without returning error). I understand that returning an error is another layer of defence but then:\r\n\r\nWe can't validate the data value ahead of time because it is being loaded from stellar-core's database.  It is an external system to horizon and can contain anything as far as horizon is concerned.  stellar-core could introduce a bug that corrupts that field; we have no way of ensuring that data is and will always be valid.  It isn't the same as the history tables, which are logically owned by horizon since horizon is populating them.  I don't know how more plainly to put this and honestly am baffled that you don't think we should be validating stellar-core data wherever we can.  The fewer instances of unconditional trust in an external system we have, the better.\r\n\r\n> If we should check values returned by horizon in the client code, then why don't we already check if account ids, hashes, links and other things I listed are valid? We should also advice developers to check the full history of sender account when a payment arrives because horizon can send incorrect value because of a bug.\r\n\r\n\r\nYou are treating this like its one or the other. Either validate everything or validate nothing.  That's not the case:  whether we choose to validate something when it is loaded from outside the process should be answered on a case by case basis.  Here's some of the questions you should be asking yourself:\r\n\r\n- Is it feasible to validate the value?  For example, we have no choice but to trust the balance fields from the stellar-core database.  There's no easy way that I know of the validate a balance other than contacting an independent stellar-core instance. \r\n- Can the value be validated further downstream?  If so, perhaps you should only validate it if you are operating upon it yourself.  In this case, you are stripping away the base64 so the downstream can no longer validate the value.\r\n- Is the value produced by your application? If so, then you can often get good enough assurance that a value will be value by using your test suite and a set of validations that run when you write data to the database.  ActiveRecord gets by pretty well using this model, but even still you see database administrators arguing for check constraints on columns to enforce the data model.\r\n- If the value is external, was it provided to you unasked (via a client http request, for example)?  If so, IMO, you _must_ validate it in some way before further operating on the value.  SQL injection lives here.\r\n\r\nThere are more I can go into, but I think I've made my point.\r\n\r\nHere's the bottom line, IMO:\r\n\r\nIf you cannot prove a value's validity, then the core question you should be asking concerns your apetite for risk.  Are you comfortable with the possible outcomes should the value in question value be corrupted?\r\n\r\nIn my opinion, we should trust but verify stellar-core's work where we can.  When I import data, I ensure the ledger chain is unbroken.  When I decode XDR data, I ensure we consume all of the data so that our confidence that the decoded value is genuine is raised.  The coverage isn't complete for sure, but the goal remains.\r\n\r\n---\r\n\r\nLet me respond directly to you questions:\r\n\r\n> then why don't we already check if account ids, hashes, links and other things I listed are valid?\r\n\r\nWhy don't we validate account ids?  Because they're opaque values most of the time that we just pass on to the next consumer.  The horizon client can validate them if they need to.  Just as when we need to, we do.\r\n\r\nHashes?  Same thing.\r\n\r\nLinks?  We do validate them:  https://github.com/stellar/horizon/blob/master/src/github.com/stellar/horizon/render/hal/link_builder.go#L46\r\n\r\nPosted transactions?  We do validate them: https://github.com/stellar/horizon/blob/master/src/github.com/stellar/horizon/txsub/system.go#L93\r\n\r\n---\r\n\r\nI wish I had an easy answer to give you, but there simply isn't one, IMO.  We are building sensitive software and we should stive to make invalid data impossible, barring that we should ensure that the risk posed is minimal or can be mitigated downstream.","created_at":"2017-02-16T00:23:01Z","updated_at":"2017-02-16T00:23:01Z","html_url":"https://github.com/stellar/horizon/pull/339#discussion_r101418588","pull_request_url":"https://api.github.com/repos/stellar/horizon/pulls/339","_links":{"self":{"href":"https://api.github.com/repos/stellar/horizon/pulls/comments/101418588"},"html":{"href":"https://github.com/stellar/horizon/pull/339#discussion_r101418588"},"pull_request":{"href":"https://api.github.com/repos/stellar/horizon/pulls/339"}}},"pull_request":{"url":"https://api.github.com/repos/stellar/horizon/pulls/339","id":106126077,"html_url":"https://github.com/stellar/horizon/pull/339","diff_url":"https://github.com/stellar/horizon/pull/339.diff","patch_url":"https://github.com/stellar/horizon/pull/339.patch","issue_url":"https://api.github.com/repos/stellar/horizon/issues/339","number":339,"state":"open","locked":false,"title":"Account.GetData","user":{"login":"bartekn","id":464938,"avatar_url":"https://avatars.githubusercontent.com/u/464938?v=3","gravatar_id":"","url":"https://api.github.com/users/bartekn","html_url":"https://github.com/bartekn","followers_url":"https://api.github.com/users/bartekn/followers","following_url":"https://api.github.com/users/bartekn/following{/other_user}","gists_url":"https://api.github.com/users/bartekn/gists{/gist_id}","starred_url":"https://api.github.com/users/bartekn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bartekn/subscriptions","organizations_url":"https://api.github.com/users/bartekn/orgs","repos_url":"https://api.github.com/users/bartekn/repos","events_url":"https://api.github.com/users/bartekn/events{/privacy}","received_events_url":"https://api.github.com/users/bartekn/received_events","type":"User","site_admin":false},"body":"Will be copied to https://github.com/stellar/go/pull/37.","created_at":"2017-02-14T15:21:32Z","updated_at":"2017-02-16T00:23:01Z","closed_at":null,"merged_at":null,"merge_commit_sha":"0c4696e724ae4b7bb077a8705363e90684d0f368","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/stellar/horizon/pulls/339/commits","review_comments_url":"https://api.github.com/repos/stellar/horizon/pulls/339/comments","review_comment_url":"https://api.github.com/repos/stellar/horizon/pulls/comments{/number}","comments_url":"https://api.github.com/repos/stellar/horizon/issues/339/comments","statuses_url":"https://api.github.com/repos/stellar/horizon/statuses/04d7fae802557a909ae045eb1c164d06e4f1142a","head":{"label":"stellar:account-get-data","ref":"account-get-data","sha":"04d7fae802557a909ae045eb1c164d06e4f1142a","user":{"login":"stellar","id":7386716,"avatar_url":"https://avatars.githubusercontent.com/u/7386716?v=3","gravatar_id":"","url":"https://api.github.com/users/stellar","html_url":"https://github.com/stellar","followers_url":"https://api.github.com/users/stellar/followers","following_url":"https://api.github.com/users/stellar/following{/other_user}","gists_url":"https://api.github.com/users/stellar/gists{/gist_id}","starred_url":"https://api.github.com/users/stellar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stellar/subscriptions","organizations_url":"https://api.github.com/users/stellar/orgs","repos_url":"https://api.github.com/users/stellar/repos","events_url":"https://api.github.com/users/stellar/events{/privacy}","received_events_url":"https://api.github.com/users/stellar/received_events","type":"Organization","site_admin":false},"repo":{"id":34413610,"name":"horizon","full_name":"stellar/horizon","owner":{"login":"stellar","id":7386716,"avatar_url":"https://avatars.githubusercontent.com/u/7386716?v=3","gravatar_id":"","url":"https://api.github.com/users/stellar","html_url":"https://github.com/stellar","followers_url":"https://api.github.com/users/stellar/followers","following_url":"https://api.github.com/users/stellar/following{/other_user}","gists_url":"https://api.github.com/users/stellar/gists{/gist_id}","starred_url":"https://api.github.com/users/stellar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stellar/subscriptions","organizations_url":"https://api.github.com/users/stellar/orgs","repos_url":"https://api.github.com/users/stellar/repos","events_url":"https://api.github.com/users/stellar/events{/privacy}","received_events_url":"https://api.github.com/users/stellar/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/stellar/horizon","description":"Horizon is the client facing HTTP API server for the Stellar ecosystem","fork":false,"url":"https://api.github.com/repos/stellar/horizon","forks_url":"https://api.github.com/repos/stellar/horizon/forks","keys_url":"https://api.github.com/repos/stellar/horizon/keys{/key_id}","collaborators_url":"https://api.github.com/repos/stellar/horizon/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/stellar/horizon/teams","hooks_url":"https://api.github.com/repos/stellar/horizon/hooks","issue_events_url":"https://api.github.com/repos/stellar/horizon/issues/events{/number}","events_url":"https://api.github.com/repos/stellar/horizon/events","assignees_url":"https://api.github.com/repos/stellar/horizon/assignees{/user}","branches_url":"https://api.github.com/repos/stellar/horizon/branches{/branch}","tags_url":"https://api.github.com/repos/stellar/horizon/tags","blobs_url":"https://api.github.com/repos/stellar/horizon/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/stellar/horizon/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/stellar/horizon/git/refs{/sha}","trees_url":"https://api.github.com/repos/stellar/horizon/git/trees{/sha}","statuses_url":"https://api.github.com/repos/stellar/horizon/statuses/{sha}","languages_url":"https://api.github.com/repos/stellar/horizon/languages","stargazers_url":"https://api.github.com/repos/stellar/horizon/stargazers","contributors_url":"https://api.github.com/repos/stellar/horizon/contributors","subscribers_url":"https://api.github.com/repos/stellar/horizon/subscribers","subscription_url":"https://api.github.com/repos/stellar/horizon/subscription","commits_url":"https://api.github.com/repos/stellar/horizon/commits{/sha}","git_commits_url":"https://api.github.com/repos/stellar/horizon/git/commits{/sha}","comments_url":"https://api.github.com/repos/stellar/horizon/comments{/number}","issue_comment_url":"https://api.github.com/repos/stellar/horizon/issues/comments{/number}","contents_url":"https://api.github.com/repos/stellar/horizon/contents/{+path}","compare_url":"https://api.github.com/repos/stellar/horizon/compare/{base}...{head}","merges_url":"https://api.github.com/repos/stellar/horizon/merges","archive_url":"https://api.github.com/repos/stellar/horizon/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/stellar/horizon/downloads","issues_url":"https://api.github.com/repos/stellar/horizon/issues{/number}","pulls_url":"https://api.github.com/repos/stellar/horizon/pulls{/number}","milestones_url":"https://api.github.com/repos/stellar/horizon/milestones{/number}","notifications_url":"https://api.github.com/repos/stellar/horizon/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/stellar/horizon/labels{/name}","releases_url":"https://api.github.com/repos/stellar/horizon/releases{/id}","deployments_url":"https://api.github.com/repos/stellar/horizon/deployments","created_at":"2015-04-22T20:18:21Z","updated_at":"2017-01-27T14:46:32Z","pushed_at":"2017-02-14T15:21:33Z","git_url":"git://github.com/stellar/horizon.git","ssh_url":"git@github.com:stellar/horizon.git","clone_url":"https://github.com/stellar/horizon.git","svn_url":"https://github.com/stellar/horizon","homepage":"","size":10710,"stargazers_count":37,"watchers_count":37,"language":"Go","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":28,"mirror_url":null,"open_issues_count":39,"forks":28,"open_issues":39,"watchers":37,"default_branch":"master"}},"base":{"label":"stellar:master","ref":"master","sha":"aa0fe10d01c251ffd0411148701a7f4cbdaf09ef","user":{"login":"stellar","id":7386716,"avatar_url":"https://avatars.githubusercontent.com/u/7386716?v=3","gravatar_id":"","url":"https://api.github.com/users/stellar","html_url":"https://github.com/stellar","followers_url":"https://api.github.com/users/stellar/followers","following_url":"https://api.github.com/users/stellar/following{/other_user}","gists_url":"https://api.github.com/users/stellar/gists{/gist_id}","starred_url":"https://api.github.com/users/stellar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stellar/subscriptions","organizations_url":"https://api.github.com/users/stellar/orgs","repos_url":"https://api.github.com/users/stellar/repos","events_url":"https://api.github.com/users/stellar/events{/privacy}","received_events_url":"https://api.github.com/users/stellar/received_events","type":"Organization","site_admin":false},"repo":{"id":34413610,"name":"horizon","full_name":"stellar/horizon","owner":{"login":"stellar","id":7386716,"avatar_url":"https://avatars.githubusercontent.com/u/7386716?v=3","gravatar_id":"","url":"https://api.github.com/users/stellar","html_url":"https://github.com/stellar","followers_url":"https://api.github.com/users/stellar/followers","following_url":"https://api.github.com/users/stellar/following{/other_user}","gists_url":"https://api.github.com/users/stellar/gists{/gist_id}","starred_url":"https://api.github.com/users/stellar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stellar/subscriptions","organizations_url":"https://api.github.com/users/stellar/orgs","repos_url":"https://api.github.com/users/stellar/repos","events_url":"https://api.github.com/users/stellar/events{/privacy}","received_events_url":"https://api.github.com/users/stellar/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/stellar/horizon","description":"Horizon is the client facing HTTP API server for the Stellar ecosystem","fork":false,"url":"https://api.github.com/repos/stellar/horizon","forks_url":"https://api.github.com/repos/stellar/horizon/forks","keys_url":"https://api.github.com/repos/stellar/horizon/keys{/key_id}","collaborators_url":"https://api.github.com/repos/stellar/horizon/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/stellar/horizon/teams","hooks_url":"https://api.github.com/repos/stellar/horizon/hooks","issue_events_url":"https://api.github.com/repos/stellar/horizon/issues/events{/number}","events_url":"https://api.github.com/repos/stellar/horizon/events","assignees_url":"https://api.github.com/repos/stellar/horizon/assignees{/user}","branches_url":"https://api.github.com/repos/stellar/horizon/branches{/branch}","tags_url":"https://api.github.com/repos/stellar/horizon/tags","blobs_url":"https://api.github.com/repos/stellar/horizon/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/stellar/horizon/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/stellar/horizon/git/refs{/sha}","trees_url":"https://api.github.com/repos/stellar/horizon/git/trees{/sha}","statuses_url":"https://api.github.com/repos/stellar/horizon/statuses/{sha}","languages_url":"https://api.github.com/repos/stellar/horizon/languages","stargazers_url":"https://api.github.com/repos/stellar/horizon/stargazers","contributors_url":"https://api.github.com/repos/stellar/horizon/contributors","subscribers_url":"https://api.github.com/repos/stellar/horizon/subscribers","subscription_url":"https://api.github.com/repos/stellar/horizon/subscription","commits_url":"https://api.github.com/repos/stellar/horizon/commits{/sha}","git_commits_url":"https://api.github.com/repos/stellar/horizon/git/commits{/sha}","comments_url":"https://api.github.com/repos/stellar/horizon/comments{/number}","issue_comment_url":"https://api.github.com/repos/stellar/horizon/issues/comments{/number}","contents_url":"https://api.github.com/repos/stellar/horizon/contents/{+path}","compare_url":"https://api.github.com/repos/stellar/horizon/compare/{base}...{head}","merges_url":"https://api.github.com/repos/stellar/horizon/merges","archive_url":"https://api.github.com/repos/stellar/horizon/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/stellar/horizon/downloads","issues_url":"https://api.github.com/repos/stellar/horizon/issues{/number}","pulls_url":"https://api.github.com/repos/stellar/horizon/pulls{/number}","milestones_url":"https://api.github.com/repos/stellar/horizon/milestones{/number}","notifications_url":"https://api.github.com/repos/stellar/horizon/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/stellar/horizon/labels{/name}","releases_url":"https://api.github.com/repos/stellar/horizon/releases{/id}","deployments_url":"https://api.github.com/repos/stellar/horizon/deployments","created_at":"2015-04-22T20:18:21Z","updated_at":"2017-01-27T14:46:32Z","pushed_at":"2017-02-14T15:21:33Z","git_url":"git://github.com/stellar/horizon.git","ssh_url":"git@github.com:stellar/horizon.git","clone_url":"https://github.com/stellar/horizon.git","svn_url":"https://github.com/stellar/horizon","homepage":"","size":10710,"stargazers_count":37,"watchers_count":37,"language":"Go","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":28,"mirror_url":null,"open_issues_count":39,"forks":28,"open_issues":39,"watchers":37,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/stellar/horizon/pulls/339"},"html":{"href":"https://github.com/stellar/horizon/pull/339"},"issue":{"href":"https://api.github.com/repos/stellar/horizon/issues/339"},"comments":{"href":"https://api.github.com/repos/stellar/horizon/issues/339/comments"},"review_comments":{"href":"https://api.github.com/repos/stellar/horizon/pulls/339/comments"},"review_comment":{"href":"https://api.github.com/repos/stellar/horizon/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/stellar/horizon/pulls/339/commits"},"statuses":{"href":"https://api.github.com/repos/stellar/horizon/statuses/04d7fae802557a909ae045eb1c164d06e4f1142a"}}}}
{"actor":{"display_login":"nullstyle"}}