-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add origin restriction to session token #46
Merged
Merged
Changes from 34 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
b0869a8
basic origin restriction
KirillDogadin-std 09e5402
comma separation logic for initialization
KirillDogadin-std 46037cd
support multiple allowed origins during validation
KirillDogadin-std e6202a1
add tests
KirillDogadin-std a8e1f8d
use wildcard
KirillDogadin-std f80b5f0
lint
KirillDogadin-std cf077ce
lint
KirillDogadin-std e382f45
default value on frontend
KirillDogadin-std af418f2
fix test
KirillDogadin-std 5ce4fdf
improve tests
KirillDogadin-std 568db2f
lint
KirillDogadin-std 962e335
add test
KirillDogadin-std c5a490e
move logic to helper
KirillDogadin-std 5757242
move throwing into helper
KirillDogadin-std 6908ff3
rename
KirillDogadin-std c1818fa
lint
KirillDogadin-std 3a62c76
table works
KirillDogadin-std ae91a76
more optimal list matcher
KirillDogadin-std 6a9899d
trim instead of invalidation
KirillDogadin-std d3076b6
throw error in validator
KirillDogadin-std 0ed5666
lint
KirillDogadin-std 61b47e6
move logic
KirillDogadin-std ff633b7
return early
KirillDogadin-std c0cf518
rename
KirillDogadin-std 552bc7b
origin header is in env
KirillDogadin-std f138154
lint
KirillDogadin-std 64528b6
add the origin into the readme
KirillDogadin-std e571847
add env var to kube values
KirillDogadin-std ed489e5
fix frontend
KirillDogadin-std cb5205c
adjust the readme
KirillDogadin-std 7c3ef05
rename func
KirillDogadin-std d9a0d58
rename and move things
KirillDogadin-std 07d968f
rename the table col
KirillDogadin-std 6fa26ec
add comment to prisma
KirillDogadin-std 12a77a8
remove env from readme and kube values
KirillDogadin-std bceee56
record origin post factum
KirillDogadin-std eae6228
lint
KirillDogadin-std 700832b
Merge branch 'main' into origin-restriction-session
KirillDogadin-std File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
import type { PrismaClient, Prisma } from '@prisma/client'; | ||
import { randomUUID } from 'crypto'; | ||
import { GraphQLError } from 'graphql'; | ||
import wildcard from 'wildcard-match'; | ||
import { token as tokenUtils } from '../../helpers'; | ||
|
||
function parseOriginMarkup(originParam: string): string { | ||
if (originParam === '*') { | ||
return '*'; | ||
} | ||
const trimmedOriginParam = originParam.trim(); | ||
const origins = trimmedOriginParam.split(',').map((origin) => origin.trim()); | ||
origins.forEach((origin) => { | ||
if (!origin.startsWith('http://') && !origin.startsWith('https://')) { | ||
throw new GraphQLError("Origin must start with 'http://' or 'https://'", { | ||
extensions: { code: 'INVALID_ORIGIN_PROTOCOL' }, | ||
}); | ||
} | ||
}); | ||
return origins.join(','); | ||
} | ||
|
||
export function validateOriginAgainstAllowed( | ||
allowedOrigins: string, | ||
originReceived?: string, | ||
) { | ||
if (allowedOrigins === '*') { | ||
return; | ||
} | ||
if (!originReceived) { | ||
throw new GraphQLError('Origin not provided', { | ||
extensions: { code: 'ORIGIN_HEADER_MISSING' }, | ||
}); | ||
} | ||
const allowedOriginsSplit = allowedOrigins.split(','); | ||
if (!wildcard(allowedOriginsSplit)(originReceived)) { | ||
throw new GraphQLError('Access denied due to origin restriction', { | ||
extensions: { code: 'ORIGIN_FORBIDDEN' }, | ||
}); | ||
} | ||
} | ||
|
||
async function newSession( | ||
prisma: PrismaClient, | ||
session: Prisma.SessionCreateInput, | ||
) { | ||
return prisma.session.create({ | ||
data: session, | ||
}); | ||
} | ||
|
||
export const generateTokenAndSession = async ( | ||
prisma: PrismaClient, | ||
userId: string, | ||
session: { expiryDurationSeconds?: number | null; name: string; allowedOrigins: string }, | ||
isUserCreated: boolean = false, | ||
) => { | ||
const createId = randomUUID(); | ||
const createdToken = tokenUtils.generate(createId, session.expiryDurationSeconds); | ||
const expiryDate = tokenUtils.getExpiryDateFromToken(createdToken); | ||
const formattedToken = tokenUtils.format(createdToken); | ||
const parsedAllowedOrigins = parseOriginMarkup(session.allowedOrigins); | ||
const createData = { | ||
allowedOrigins: parsedAllowedOrigins, | ||
name: session.name, | ||
referenceExpiryDate: expiryDate, | ||
id: createId, | ||
referenceTokenId: formattedToken, | ||
isUserCreated, | ||
creator: { | ||
connect: { | ||
id: userId, | ||
}, | ||
}, | ||
}; | ||
const createdSession = await newSession(prisma, createData); | ||
return { | ||
token: createdToken, | ||
session: createdSession, | ||
}; | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valiafetisov don't have prior xp on this one, googling did not yield me a definite answer.
Is relying on the origin header safe? can't it be forged? e.g. in tests i manage to put any origin i want and hence i imagine that this approach is not safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header can not be changed by the JavaScript running in the browser, but can be set in many other environments (like curl). It is sufficient for the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related question to @DeusAvalon. Is it enough to just
req.get('Origin')
on the application level to get the actual origin (notfrontend:3000
) or should ingress be additionally configured to pass proper header to the application? Or does it come via something likex-origin
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im pretty sure
req.get('Origin')
on Application level should give the correct Origin (actual Ingress URL) as the Ingress does not strip/replace any headers in our current configuration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, then what header contains internal
frontend:3000
? And what isproxy-add-original-uri-header
for? (related issue discussing the subject)If possible, I would ask kirill to enable staging deployments on this branch to validate that it indeed works, but as the repo wasn't transferred yet, we would need to test (and potentially fix) it after this PR is merged, via our fork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm i see, yeah im not 100% sure either i would need to actually deploy a service and check which headers are being parsed to the service.
But the staging deployment could actually be done by me manually deploying it based on this branch. This way we can pre-validate if its actually working. If this would work for you i would organize this tmrw with @KirillDogadin-std together