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

chore(validation): add validation for all routes #1854

Merged
merged 5 commits into from
Mar 20, 2024

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Mar 18, 2024

Problem

Some of our routes don't have validation - this leads to security vulnerabilities where path traversal attacks can be done.

Solution

Validate routes based on teh following criteria (observed behaviour):

  1. top level sites/resource categories/resource rooms will not have special characters; spaces will get converted to - in the url so it's ok if we validate alphanumeric + -. do note that non-ascii characters (chi/tamil) might have issues...
  2. for filenames, we have the extension + we allow non-ascii characters (chi/tamil) so we will only check for special characters (inclusive of .)
  3. for media folders, we allow / (encoded) so we have to split and validate each one separately .___.

Tests

NOTE: refer to this doc for special chars

Sitename/resource name/collection

  • ensure that you cannot create any of these with a special character
  • create one of these (less sitename) with a space + a dash
  • ensure that you can access them (note that hte url converts space to a dash)

filename

  • create a filename with the allowed special characters (given in the doc)
  • create a filename w/ chi/tamil characters (non-ascii)
  • ensure that you can access those pages

sub-collection + resource category

  • ensure that you cannot create a sub-collection with teh disallowed special characters
  • create a sub-collection with the allowed special characters (see doc) + ensure that the sub-collection has a dash + a space
  • ensure that you can access said sub-collection

media
note that for media stuff, we can create nested folders and these folders are encoded using %2f

  • ensure that you cannot create a media folder with teh disallowed special characters
  • ensure that you can create a media folder with the allowed special characters
  • ensure that you can upload images/files with special characters and that the special characters are renamed to _
  • ensure that you can rename the images/files to allowed special characters
  • ensure that you cannot rename the images/files to disallowed special characters

@seaerchin seaerchin changed the title Chore/add validation chore(validation): add validation for all routes Mar 18, 2024
@seaerchin seaerchin marked this pull request as ready for review March 18, 2024 06:50
@seaerchin seaerchin requested a review from a team March 18, 2024 06:50
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

hmm my only concern here is testing.
this pr could potentially do a 404 in a vaild path
could you add some basic testing to go into all the affected path just as a sanity check that this is working as intended?

src/routing/RouteSelector.jsx Outdated Show resolved Hide resolved
@seaerchin
Copy link
Contributor Author

hmm my only concern here is testing. this pr could potentially do a 404 in a vaild path could you add some basic testing to go into all the affected path just as a sanity check that this is working as intended?

yea, i checked on local and there's unit tests also :pog: (RouteSelector.spec.jsx).

i'll add a set of manual tests to run through during releaes so it's not just me

@seaerchin seaerchin merged commit 76bc4ac into develop Mar 20, 2024
8 checks passed
@seaerchin seaerchin deleted the chore/add-validation branch March 20, 2024 07:38
@seaerchin seaerchin mentioned this pull request Mar 21, 2024
7 tasks
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.

2 participants