{"action":"created","issue":{"url":"https://api.github.com/repos/triplea-game/triplea/issues/4379","repository_url":"https://api.github.com/repos/triplea-game/triplea","labels_url":"https://api.github.com/repos/triplea-game/triplea/issues/4379/labels{/name}","comments_url":"https://api.github.com/repos/triplea-game/triplea/issues/4379/comments","events_url":"https://api.github.com/repos/triplea-game/triplea/issues/4379/events","html_url":"https://github.com/triplea-game/triplea/pull/4379","id":384050167,"node_id":"MDExOlB1bGxSZXF1ZXN0MjMzMzU3NzM5","number":4379,"title":"ProtectedStringClientSetting: Require owner to scrub char array","user":{"login":"ssoloff","id":4826349,"node_id":"MDQ6VXNlcjQ4MjYzNDk=","avatar_url":"https://avatars2.githubusercontent.com/u/4826349?v=4","gravatar_id":"","url":"https://api.github.com/users/ssoloff","html_url":"https://github.com/ssoloff","followers_url":"https://api.github.com/users/ssoloff/followers","following_url":"https://api.github.com/users/ssoloff/following{/other_user}","gists_url":"https://api.github.com/users/ssoloff/gists{/gist_id}","starred_url":"https://api.github.com/users/ssoloff/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ssoloff/subscriptions","organizations_url":"https://api.github.com/users/ssoloff/orgs","repos_url":"https://api.github.com/users/ssoloff/repos","events_url":"https://api.github.com/users/ssoloff/events{/privacy}","received_events_url":"https://api.github.com/users/ssoloff/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-11-25T05:49:44Z","updated_at":"2018-11-26T00:58:55Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/triplea-game/triplea/pulls/4379","html_url":"https://github.com/triplea-game/triplea/pull/4379","diff_url":"https://github.com/triplea-game/triplea/pull/4379.diff","patch_url":"https://github.com/triplea-game/triplea/pull/4379.patch"},"body":"## Overview\r\n\r\nFollow-up to https://github.com/triplea-game/triplea/pull/4349#discussion_r234394727.\r\n\r\nHaving `ProtectedStringClientSetting#setValue()` scrub its argument may lead to surprises because nothing in the `GameSetting#setValue()` method contract states that the argument will be modified by the implementation.  If a caller wishes to make additional use of the argument, they either need to make a copy or rearrange code to ensure all usage occurs before the call to `setValue()`.  This could lead to hard-to-track-down bugs.\r\n\r\n## Functional Changes\r\n\r\n`ProtectedStringClientSetting#setValue()` no longer scrubs the new value.  It is now the responsibility of the character array's owner to scrub it at the appropriate time.\r\n\r\n## Refactoring Changes\r\n\r\nThe `SelectionComponent#readValues()` method wasn't amenable to scrubing sensitive values because `SaveFunction` pulls the values out of `SelectionComponent`, which forces the owner to give up ownership to `SaveFunction`, which doesn't know that one of the values may need to be scrubbed.\r\n\r\nI addressed this by inverting the dependency so that `SaveFunction` passes an abstraction to `SelectionComponent` through which the setting value can be set.  This has the following benefits:\r\n\r\n* The scope of the new setting values are completely within the new `SelectionComponent#save()` method so that they can be properly scrubbed if they happen to contain sensitive values.\r\n* New setting values can now be set in a type-safe manner due to no longer needing to store all values in a heterogeneous map.\r\n\r\n## Manual Testing Performed\r\n\r\nSmoke tested the client settings UI in both the Swing and JFX clients."},"comment":{"url":"https://api.github.com/repos/triplea-game/triplea/issues/comments/441490477","html_url":"https://github.com/triplea-game/triplea/pull/4379#issuecomment-441490477","issue_url":"https://api.github.com/repos/triplea-game/triplea/issues/4379","id":441490477,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0MTQ5MDQ3Nw==","user":{"login":"ssoloff","id":4826349,"node_id":"MDQ6VXNlcjQ4MjYzNDk=","avatar_url":"https://avatars2.githubusercontent.com/u/4826349?v=4","gravatar_id":"","url":"https://api.github.com/users/ssoloff","html_url":"https://github.com/ssoloff","followers_url":"https://api.github.com/users/ssoloff/followers","following_url":"https://api.github.com/users/ssoloff/following{/other_user}","gists_url":"https://api.github.com/users/ssoloff/gists{/gist_id}","starred_url":"https://api.github.com/users/ssoloff/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ssoloff/subscriptions","organizations_url":"https://api.github.com/users/ssoloff/orgs","repos_url":"https://api.github.com/users/ssoloff/repos","events_url":"https://api.github.com/users/ssoloff/events{/privacy}","received_events_url":"https://api.github.com/users/ssoloff/received_events","type":"User","site_admin":false},"created_at":"2018-11-26T00:58:55Z","updated_at":"2018-11-26T00:58:55Z","author_association":"MEMBER","body":"It's less about actively scraping memory and more about ensuring the sensitive data doesn't propagate beyond where it's actually needed.  When sensitive data moves about unnecessarily, it's open to accidental disclosure (e.g. via logs or other locations in the file system; #1884 is sort of related to this).\r\n\r\n> Focusing on simple code is more worthwhile IMO.\r\n\r\nIf we're going to say it's not worth our time to secure sensitive data in memory, then I would suggest that be done en masse in a separate PR, as it will affect other parts of the system beyond usage of `ProtectedStringClientSetting`.  This PR simply moves where some existing scrubbing is done to avoid a potentially hard-to-find bug."}}
{"actor":{"display_login":"ssoloff"}}