Skip to content

Making the regex that gets the index courses more secure - #506

Merged
OmarIthawi merged 1 commit into
appsembler/tahoe/developfrom
omar/course-api-fixup
Dec 26, 2019
Merged

Making the regex that gets the index courses more secure#506
OmarIthawi merged 1 commit into
appsembler/tahoe/developfrom
omar/course-api-fixup

Conversation

@OmarIthawi

Copy link
Copy Markdown

So https://red.staging-tahoe.appsembler.com/api/courses/v1/courses/ won't show up redislabs courses.

It used to match redislabs orgs as if it was red. https://github.com/edx/edx-platform/pull/21220 fixes that issues and this pull request cherry pick it.

@OmarIthawi
OmarIthawi requested review from iamjazzar, jazztpt, johnbaldwin, melvinsoft and thraxil and removed request for jazztpt December 26, 2019 16:52

@thraxil thraxil left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks more secure. I'm not sure if using a regex match in the SQL query is what we ought to be doing in general. It seems like the kind of thing that could lead to an injection or at least DoS issue someday.

If we're just finding courses that have an org that's in a list, it seems like some combination of __in and __iexact is more what we want. Something like what's discussed here: https://stackoverflow.com/questions/14907525/how-can-i-chain-djangos-in-and-iexact-queryset-field-lookups/14908214

We might also step back and ask why the courses just have the org as a string that we then match on rather than a proper model and foreign key construction.

@OmarIthawi

OmarIthawi commented Dec 26, 2019

Copy link
Copy Markdown
Author

You're right @thraxil, but that would be swimming against the upstream (https://github.com/edx/edx-platform/pull/21220):

I don't think we can afford it for this bit.

@OmarIthawi
OmarIthawi merged commit 0db9906 into appsembler/tahoe/develop Dec 26, 2019
@OmarIthawi
OmarIthawi deleted the omar/course-api-fixup branch December 26, 2019 18:43
@OmarIthawi

OmarIthawi commented Dec 30, 2019

Copy link
Copy Markdown
Author

Thanks for the original suggestion @thraxil I've made an upstream pull request to refactor it:

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