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: more permissive type assertions #3692

Merged
merged 4 commits into from
May 18, 2021

Conversation

joshgummersall
Copy link
Contributor

Fixes #3684

@joshgummersall joshgummersall requested review from stevengum and a team as code owners May 17, 2021 16:38
@coveralls
Copy link

coveralls commented May 17, 2021

Pull Request Test Coverage Report for Build 854251439

  • 5 of 17 (29.41%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 85.451%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botbuilder-applicationinsights/src/applicationInsightsTelemetryClient.ts 0 1 0.0%
libraries/botbuilder-azure/src/cosmosDbPartitionedStorage.ts 0 1 0.0%
libraries/botbuilder-azure-blobs/src/blobsStorage.ts 2 6 33.33%
libraries/botbuilder-azure-blobs/src/blobsTranscriptStore.ts 3 9 33.33%
Totals Coverage Status
Change from base Build 851910644: -0.01%
Covered Lines: 19236
Relevant Lines: 21414

💛 - Coveralls

[activity.channelId, activity.conversation.id, `${formatTicks(activity.timestamp)}-${activity.id}.json`].join(
'/'
)
[activity.channelId, activity.conversation.id, `${formatTicks(timestamp)}-${activity.id}.json`].join('/')
Copy link
Member

Choose a reason for hiding this comment

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

Not something for this PR, but in v5 the behavior around generating keys needs to be locked down and inputs need to be validated.

Are null, undefined, empty strings or whitespace-only strings valid for conversation IDs?

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

LGTM

stevengum and others added 3 commits May 18, 2021 10:23
These can end up in serialized JSON transcripts as well, so to limit the chance of including connection strings or other
secrets we should restrict their serialized forms.
@joshgummersall joshgummersall force-pushed the jpg/blobs-permissive-assertions branch from 62b1b5f to d406699 Compare May 18, 2021 18:09
@joshgummersall
Copy link
Contributor Author

In lieu of Github Actions (current status is degraded perf), I've manually run the same CI suite via Dev Ops.

@joshgummersall
Copy link
Contributor Author

Merging as ADO CI passed.

@joshgummersall joshgummersall merged commit 6437084 into main May 18, 2021
@joshgummersall joshgummersall deleted the jpg/blobs-permissive-assertions branch May 18, 2021 18:59
joshgummersall added a commit that referenced this pull request May 18, 2021
* fix: more permissive type assertions

Fixes #3684

* fix: json serialization cycles (#3693)

See microsoft/BotFramework-Composer#7854 for more details.

* fix: limit serialized form of storage types

These can end up in serialized JSON transcripts as well, so to limit the chance of including connection strings or other
secrets we should restrict their serialized forms.

Co-authored-by: Steven Gum <[email protected]>
joshgummersall added a commit that referenced this pull request May 18, 2021
* fix: more permissive type assertions

Fixes #3684

* fix: json serialization cycles (#3693)

See microsoft/BotFramework-Composer#7854 for more details.

* fix: limit serialized form of storage types

These can end up in serialized JSON transcripts as well, so to limit the chance of including connection strings or other
secrets we should restrict their serialized forms.

Co-authored-by: Steven Gum <[email protected]>
joshgummersall added a commit that referenced this pull request May 18, 2021
* fix: more permissive type assertions (#3692)

* fix: more permissive type assertions

Fixes #3684

* fix: json serialization cycles (#3693)

See microsoft/BotFramework-Composer#7854 for more details.

* fix: limit serialized form of storage types

These can end up in serialized JSON transcripts as well, so to limit the chance of including connection strings or other
secrets we should restrict their serialized forms.

Co-authored-by: Steven Gum <[email protected]>

* fix: two typos (#3696)

Co-authored-by: Steven Gum <[email protected]>
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.

Transcript logger middleware throwing error with botbuilder-azure-blobs library
4 participants