-
Notifications
You must be signed in to change notification settings - Fork 24
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(ws): reject invalid requests #139
base: notebooks-v2
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@Yael-F can you please rebase this? |
5ec5d4d
to
9ac71ad
Compare
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.
just added a comment regarding the valid name. We will need to improve it!
workspaces/backend/api/validation.go
Outdated
|
||
// NonASCIIValidator checks if a given string contains only ASCII characters. | ||
func NonASCIIValidator(param string) error { | ||
if utf8.ValidString(param) && len(param) == len([]rune(param)) { |
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.
@Yael-F when I was reviewing I wondered if the specification that we wrote is enough. I believe ValidString only validates if it's a valid utf8 string, but k8s resources needs to be:
- Length: Must be 1-253 characters long.
-Allowed Characters:
• Must consist of:
• Lowercase letters (a-z)
• Numbers (0-9)
• Hyphens (-)
• Periods (.) - Start and End:
• Must begin and end with an alphanumeric character (a-z or 0-9).
@thesuperzapper can you confirm?
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.
@ederign Thanks for the review, updated the code, if you can pls take a look
and @Yael-F sorry for the delay on this one. as you know, I was on a long pto! :) |
9ac71ad
to
4712ee0
Compare
Signed-off-by: Yael <[email protected]>
4712ee0
to
a5c1b40
Compare
Fixes #129 by implementing validation for names and namespaces.
Summary of Changes
Validation Enhancements:
Ensures names/namespaces extracted from URL parameters or request payloads are:
New Validation Module:
Introduced functions for validation:
NonASCIIValidator
LengthValidator
ValidateKubernetesResourceName
Handler Updates:
Applied validation to the following routes:
GET /workspacekind/:name
GET /workspaces/:namespace/:name
GET /workspaces/:namespace
POST /workspaces/:namespace
Testing:
Updated test files (workspaces_handler_test.go, workspacekinds_handler_test.go) to ensure unsupported fields trigger validation errors.