feat(auth): validate content-digest header #777
Conversation
| runWhen: (config) => | ||
| config.method === 'post' || !!config.headers['Authorization'] |
There was a problem hiding this comment.
could maybe just be a separate interceptor to avoid the OR?
Additionally, maybe config.method.toLower/UpperCase() to be safe?
| (!sigInputComponents.includes('content-digest') || | ||
| !verifyContentDigest( | ||
| JSON.stringify(ctx.request.body), | ||
| ctx.headers['content-digest'] as string |
There was a problem hiding this comment.
would probably err on the side of caution and check !!ctx.headers['content-digest'], verifyContentDigest might end up throwing an error that could bubble up in a weird way
There was a problem hiding this comment.
Might be good to pull out
(ctx.request.body &&
Object.keys(ctx.request.body).length > 0 &&
(!sigInputComponents.includes('content-digest') ||
!verifyContentDigest(
JSON.stringify(ctx.request.body),
ctx.headers['content-digest'] as string
)))into its own fn/var
| @@ -21,6 +21,7 @@ | |||
| "@koa/router": "^12.0.0", | |||
| "ajv": "^8.11.0", | |||
| "axios": "^0.27.2", | |||
| "httpbis-digest-headers": "github:interledger/httpbis-digest-headers", | |||
There was a problem hiding this comment.
how does this install work?
There was a problem hiding this comment.
https://pnpm.io/cli/add#install-from-git-repository
I don't think https://github.com/interledger/httpbis-digest-headers has been published to npm yet
6c138b0 to
1c9b3ab
Compare
1c9b3ab to
f60a2e2
Compare
| axiosInstance.interceptors.request.use(interceptor, null, { | ||
| runWhen: (config) => | ||
| config.method.toLowerCase() === 'post' || | ||
| !!config.headers['Authorization'] | ||
| }) | ||
| axiosInstance.interceptors.request.use(interceptor, null, { | ||
| runWhen: (config) => !!config.headers['Authorization'] | ||
| }) |
There was a problem hiding this comment.
I think the interceptor would be run twice for POST requests with Authorization
https://github.com/axios/axios#multiple-interceptors
There was a problem hiding this comment.
Sounds like we should just keep using one interceptor then
Changes proposed in this pull request
open-paymentsauthContext
Checklist
fixes #number