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 #324 Responding with 401 status, not 500 for signature verification failures #362

Merged
merged 3 commits into from
Jan 22, 2020

Conversation

seratch
Copy link
Member

@seratch seratch commented Dec 25, 2019

Summary

This pull request fixes #324 - refer to the issue for details.

Requirements (place an x in each [ ])

@seratch seratch self-assigned this Dec 25, 2019
@seratch seratch added enhancement M-T: A feature request for new functionality semver:patch labels Dec 25, 2019
@codecov
Copy link

codecov bot commented Dec 25, 2019

Codecov Report

Merging #362 into master will decrease coverage by 0.48%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
- Coverage    83.2%   82.71%   -0.49%     
==========================================
  Files           7        7              
  Lines         500      509       +9     
  Branches      146      148       +2     
==========================================
+ Hits          416      421       +5     
- Misses         55       57       +2     
- Partials       29       31       +2
Impacted Files Coverage Δ
src/ExpressReceiver.ts 68.14% <73.33%> (-1.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d59cf7...303c168. Read the comment docs.

@seratch seratch force-pushed the issue-324 branch 2 times, most recently from 303c168 to 4fc253b Compare January 4, 2020 01:47
@codecov-io
Copy link

codecov-io commented Jan 4, 2020

Codecov Report

Merging #362 into master will decrease coverage by 0.16%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #362      +/-   ##
=========================================
- Coverage   83.46%   83.3%   -0.17%     
=========================================
  Files           7       7              
  Lines         508     509       +1     
  Branches      147     147              
=========================================
  Hits          424     424              
- Misses         55      56       +1     
  Partials       29      29
Impacted Files Coverage Δ
src/ExpressReceiver.ts 70.79% <0%> (-0.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f15c2ed...ca651ce. Read the comment docs.

}
};
}

function logError(logger: Logger, message: string, error: any): void {
const logMessage = ('code' in error)
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, all the possible error values have code but this is testing the existence just in case.

Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

LGTM. Logic looks sound. I agree with @aoberoi assessment over at #324 (comment) that in semver land this should be a major, but since the chance of it actually breaking code is so minimal, we should treat it as a minor.

@seratch seratch merged commit 6117d4e into slackapi:master Jan 22, 2020
@seratch seratch deleted the issue-324 branch January 22, 2020 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Responding with 401 status, not 500 for signature verification failures
3 participants