Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove sensitive information from agent config toJSON() method #1112

Conversation

jimezesinachi
Copy link
Contributor

Signed-off-by: Jim Ezesinachi jim@animo.id

Signed-off-by: Jim Ezesinachi <jim@animo.id>
@jimezesinachi jimezesinachi requested a review from a team as a code owner November 21, 2022 11:15
Signed-off-by: Jim Ezesinachi <jim@animo.id>
Signed-off-by: Jim Ezesinachi <jim@animo.id>
@jimezesinachi
Copy link
Contributor Author

All checks passed! @blu3beri

Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning them as undefined might be confusing as you won't know whether they are actually supplied.

packages/core/src/agent/AgentConfig.ts Outdated Show resolved Hide resolved
jimezesinachi and others added 2 commits November 21, 2022 15:30
Co-authored-by: Berend Sliedrecht <61358536+blu3beri@users.noreply.github.com>
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Signed-off-by: Jim Ezesinachi <jim@animo.id>
@berendsliedrecht
Copy link
Contributor

@rapaktech Prettier is failing now :). Could you fix that? I can merge it afterwards.

@berendsliedrecht
Copy link
Contributor

@TimoGlastra would this be considered a breaking change? The return type of the function is quite different now, but I don't think it is a "public method"

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't call this a breaking change, if you need properties from the agent config you should do so using the class methods.

This LGTM @rapaktech. I just found another place where we log keys, could you also tackle that one in this PR? It's in the WalletApi.initialize() method (Initializing wallet xxx)

BTW -- marking this as a fix over chore, as we shouldn't have logged the sensitive info in the first place

@jimezesinachi
Copy link
Contributor Author

You got it, Timo. On it.

@jimezesinachi jimezesinachi changed the title chore: remove PII from agent config toJSON() method fix: remove PII from agent config toJSON() method Nov 22, 2022
Signed-off-by: Jim Ezesinachi <jim@animo.id>
Signed-off-by: Jim Ezesinachi <jim@animo.id>
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022

Codecov Report

Merging #1112 (00947eb) into main (6747756) will decrease coverage by 0.05%.
The diff coverage is 85.35%.

@@            Coverage Diff             @@
##             main    #1112      +/-   ##
==========================================
- Coverage   88.33%   88.28%   -0.06%     
==========================================
  Files         701      705       +4     
  Lines       16111    16399     +288     
  Branches     2597     2657      +60     
==========================================
+ Hits        14232    14478     +246     
- Misses       1872     1914      +42     
  Partials        7        7              
Impacted Files Coverage Δ
...ges/core/src/modules/common/messages/AckMessage.ts 100.00% <ø> (ø)
...ges/core/src/modules/credentials/CredentialsApi.ts 86.91% <ø> (ø)
...les/credentials/formats/CredentialFormatService.ts 100.00% <ø> (ø)
...ntials/formats/indy/IndyCredentialFormatService.ts 87.95% <0.00%> (+0.52%) ⬆️
...s/protocol/v2/handlers/V2IssueCredentialHandler.ts 96.00% <ø> (ø)
...s/protocol/v2/handlers/V2OfferCredentialHandler.ts 67.74% <ø> (ø)
...protocol/v2/handlers/V2RequestCredentialHandler.ts 96.42% <0.00%> (ø)
...ckages/core/src/modules/dids/domain/DidDocument.ts 96.47% <ø> (ø)
packages/core/src/modules/ledger/LedgerApi.ts 97.61% <ø> (ø)
...e/src/modules/proofs/formats/ProofFormatService.ts 100.00% <ø> (ø)
... and 30 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jimezesinachi
Copy link
Contributor Author

I think we're good @jakubkoci

@TimoGlastra TimoGlastra changed the title fix: remove PII from agent config toJSON() method fix: remove sensitive information from agent config toJSON() method Nov 23, 2022
@TimoGlastra TimoGlastra merged commit 427a80f into openwallet-foundation:main Nov 23, 2022
Artemkaaas pushed a commit to sicpa-dlab/aries-framework-javascript that referenced this pull request Dec 5, 2022
@jimezesinachi jimezesinachi deleted the chore/remove-pii-from-agent-config-tojson branch March 14, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants