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

feat(backend): bypass sig validation #741

Merged
merged 4 commits into from
Nov 10, 2022
Merged

Conversation

sabineschaller
Copy link
Member

@sabineschaller sabineschaller commented Nov 10, 2022

Changes proposed in this pull request

Context

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass

@github-actions github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels Nov 10, 2022
@sabineschaller sabineschaller requested a review from njlie November 10, 2022 01:51
njlie
njlie previously approved these changes Nov 10, 2022
Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

You could add tests that bypassSignatureValidation:

  • works with invalid signature
  • doesn't work with invalid token

@@ -27,7 +27,7 @@ export function createAuthMiddleware({
const token = parts[1]
if (
process.env.NODE_ENV !== 'production' &&
token === config.devAccessToken
(token === config.devAccessToken || config.bypassSignatureValidation)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the check should be moved below to wrap:

try {
if (!(await verifySigAndChallenge(grant.key.jwk, ctx))) {
ctx.throw(401, 'Invalid signature')
}
} catch (e) {
ctx.status = 401
ctx.throw(401, `Invalid signature`)
}

mkurapov
mkurapov previously approved these changes Nov 10, 2022
@sabineschaller sabineschaller dismissed stale reviews from mkurapov and njlie via 8fbd9ae November 10, 2022 02:47
Co-authored-by: Brandon Wilson <[email protected]>
try {
if (!(await verifySigAndChallenge(grant.key.jwk, ctx))) {
ctx.throw(401, 'Invalid signature')
if (!config.bypassSignatureValidation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Log might be handy when signature validation is being bypassed, could also do an early return to avoid nesting

Copy link
Contributor

Choose a reason for hiding this comment

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

could also do an early return to avoid nesting

I think we do still want the ctx.grant = grant below

@sabineschaller sabineschaller merged commit 73f573d into main Nov 10, 2022
@sabineschaller sabineschaller deleted the s2-backend-bypass-sig-val branch November 10, 2022 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants