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.

Sign up for GitHub

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

Jump to bottom

Update error msg on stale request #1503

Merged
merged 3 commits into from Jun 24, 2022
Merged

Update error msg on stale request #1503

merged 3 commits into from Jun 24, 2022

Conversation

srajiang
Copy link
Member

Summary

Fix #1502.
Tweaks error messages in case of stale request to add more detail for end-users

Requirements (place an x in each [ ])

@srajiang srajiang mentioned this pull request Jun 24, 2022
Closed
10 tasks
@srajiang srajiang added the docs M-T: Documentation work only label Jun 24, 2022
@srajiang srajiang self-assigned this Jun 24, 2022
@codecov
Copy link

codecov bot commented Jun 24, 2022 •
edited

Codecov Report

Merging #1503 (a7b42cf) into main (e44a1ff) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head a7b42cf differs from pull request most recent head cb024b0. Consider uploading reports for the commit cb024b0 to get more accurate results

@@            Coverage Diff             @@
##             main    #1503      +/-   ##
==========================================
+ Coverage   81.99%   82.00%   +0.01%     
==========================================
  Files          18       18              
  Lines        1494     1495       +1     
  Branches      435      435              
==========================================
+ Hits         1225     1226       +1     
  Misses        172      172              
  Partials       97       97
Impacted Files Coverage Δ
src/receivers/verify-request.ts 73.52% <100.00%> (+0.80%) ⬆️

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 e44a1ff...cb024b0. Read the comment docs.

seratch
seratch requested changes Jun 24, 2022
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

A minor suggestion. What do you think?

src/receivers/verify-request.ts Outdated
@@ -44,7 +44,7 @@ export function verifySlackRequest(options: SlackRequestVerificationOptions): vo

// Rule 1: Check staleness
if (requestTimestampSec < fiveMinutesAgoSec) {
throw new Error(`${verifyErrorPrefix}: stale`);
throw new Error(`${verifyErrorPrefix}: x-slack-request-timestamp must differ from system time by no more than 5 minutes or request is stale`);
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace the hard-coded "5" in the log message? Perhaps, we can add a new local variable for the minutes above in this method and embed the value in the log.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@srajiang srajiang requested a review from seratch June 24, 2022 00:35
seratch
seratch requested changes Jun 24, 2022
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks, can you check this?

src/receivers/verify-request.ts Outdated Show resolved Hide resolved
@srajiang @seratch
Update src/receivers/verify-request.ts
cb024b0 
Co-authored-by: Kazuhiro Sera <seratch@gmail.com>
@srajiang
Copy link
Member Author

srajiang commented Jun 24, 2022 •
edited

Thanks, can you check this?

@seratch - Oh yes, that's definitely better. I was going too fast! Just checked it's fine.

@srajiang srajiang requested a review from seratch June 24, 2022 00:39
seratch
seratch approved these changes Jun 24, 2022
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

I haven't checked wether this works on my local machine but if it works, looks great to me 👍

@seratch seratch added this to the 3.11.4 milestone Jun 24, 2022
@seratch seratch added the enhancement M-T: A feature request for new functionality label Jun 24, 2022
@srajiang
Copy link
Member Author

@seratch - If you mean testing how a template literal would handle injecting the number, yes it looks like no complaints - tests are passing as well.

@srajiang srajiang merged commit 7eec438 into main Jun 24, 2022
@srajiang srajiang deleted the improve-request-verification-error-msg branch June 24, 2022 00:53
@seratch seratch modified the milestones: 3.11.4, 3.12.0 Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch approved these changes

@misscoded misscoded Awaiting requested review from misscoded

@mwbrooks mwbrooks Awaiting requested review from mwbrooks

Assignees

@srajiang srajiang

Labels
docs M-T: Documentation work only enhancement M-T: A feature request for new functionality
Projects
None yet
Milestone
3.12.0
Development

Successfully merging this pull request may close these issues.

Request verification failed: Failed to verify authenticity: stale
2 participants
@srajiang @seratch