fix: request verification example code in comments not being usable #15
Conversation
VaguelySerious
commented
Nov 4, 2020
•
VaguelySerious
commented
- Fix error from missing number casting of timestamp header
- Fix code in comments/documentation not being usable due to wrong imports
- Some beauty changes
src/requests/sign-request.ts
Outdated
| * const {pick} = require('lodash') | ||
| * const {server} = require('./imaginary-server') | ||
| * | ||
| * const SECRET = process.env.SECRET | ||
| * | ||
| * server.post('/api/my-resources', (req, res) => { | ||
| * const incomingSignature = req.headers['x-contentful-signature'] | ||
| * const incomingTimestamp = req.headers['x-contentful-timestamp'] | ||
| * const incomingTimestamp = Number(req.headers['x-contentful-timestamp']) |
There was a problem hiding this comment.
This really depends on the imaginary framework, I don't it's a mistake. If we want to enforce type, then we should use Number.parseInt
There was a problem hiding this comment.
Headers are always strings, and this code will, as far as I can see, fail 100% of the time, because we pass incomingTimestamp to TimestampValidator internally which will throw if it is a string.
I will use Number.parseInt, that's fine
src/requests/utils.ts
Outdated
| return headers | ||
| } | ||
|
|
||
| export const pickHeaders = (headers: Record<string, string>, keys: string[]) => { |
There was a problem hiding this comment.
Headers are generally speaking not mandatory in our model. This should stay optional for consistency
There was a problem hiding this comment.
These if statements are unused, even if the user passes no headers, because we already validate it beforehand and pass an empty object here
src/index.ts
Outdated
| @@ -1,3 +1,3 @@ | |||
| export { getManagementToken } from './keys' | |||
| export { signRequest, verifyRequest, ContentfulHeader } from './requests' | |||
| export { Secret, CanonicalRequest, Timestamp, SignedRequestHeaders } from './requests/typings' | |||
| export { CanonicalRequest, SignedRequestHeaders } from './requests/typings' | |||
There was a problem hiding this comment.
Plus, this should be moved into the ./requests/index.ts and exported from there
There was a problem hiding this comment.
Secret and Timestamp are just string and number types, and they are not otherwise relevant to the user, so it make little sense to export them.
I agree that the other types should be exported through requests/index
|
Forgot to mention, the commit type should be |
How do I change the commit type after the fact? Force push? And since we are exporting more types, a fix version seems appropriate |
|
🎉 This PR is included in version 1.3.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
