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

Express customSanitizer needs error catching and validity checks #653

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

sei-vsarvepalli
Copy link

@sei-vsarvepalli sei-vsarvepalli commented May 3, 2022

Requirements for Contributing a Bug Fix

Identify the Bug

#609 seems to be an ongoing issue that can happen in a few scenarios.

Description of the Change

Every customsantizier for validating and publishing the roles array will

    .customSanitizer(val => {
	try {
	    return val.map(x => { return x.toUpperCase() })
	} catch(err) {
	    return false
	}
    }).custom(val => {
	if (val) {
	    return isOrgRole(val)
	}
	return Promise.reject('Value should be an array of valid roles')
    })

Alternate Designs

Do not use customSanitizer method at all, just use the isOrgRole and isUserRole and use the native toUpperCase method while adding/managing roles.

OR

Use the customSanitizer as it is done tin the toDate method in

function toDate (val) {
let value = val.match(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?(|Z|((-|\+|\s)\d{2}:\d{2}))$/)
let result
if (value) {
value[0] = value[0].replace(' ', '+') // Re-add literal '+' which was stripped
result = new Date(value[0])
} else {
value = val.match(/^\d{4}-\d{2}-\d{2}$/)
if (value) {
result = new Date(`${value[0]}T00:00:00.000+00:00`)
} else {
result = null
}
}
return result
}

Possible Drawbacks

None I can think of.

Verification Process

Testing with a local instance with fuzzing input for active_roles

Release Notes

This release fixes bug #609 which can lead to unexpected 500 errors and revealing the lack of input validation.

@sei-vsarvepalli sei-vsarvepalli marked this pull request as ready for review May 3, 2022 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant