Conversation
…port and remixer.
| async function appendPressbooksJobMessages(jobID: string, messages: string[]) { | ||
| if (!messages.length) return; | ||
| await PressbooksImportJob.updateOne( | ||
| { jobID }, |
There was a problem hiding this comment.
NoSQL injection attack possible - critical severity
Query injection attacks are possible if users can pass objects instead of strings to query functions such as findOne.
By injecting query operators attackers can control the behavior of the query, allowing them to bypass access controls and extract unauthorized data. Consider the attack payload ?user_id[$ne]=5: if the user_id query parameter is passed to the query function without validation or casting its type, an attacker can pass {$ne: 5} instead of an integer to the query. {$ne: 5} uses the 'not equal to' operator to access data of other users.
While this vulnerability is known as NoSQL injection, relational databases (mysql, postgres) are also vulnerable to this attack if the query library offers a NoSQL-like API and supports string-typed query operators. Examples include prisma and sequelize versions prior to 4.12.0.
Show fix
Remediation: User input should be validated (e.g. with class-validator, zod or joi) or sanitized (e.g. with mongo-sanitize). Alternatively cast request parameters to their expected type or use the $eq operator to block object injection. You can also Autofix all instances of NoSQL injection by installing Zen for Node.js.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…xer API endpoints
jakeaturner
left a comment
There was a problem hiding this comment.
Hey @yghaemi tons of great work here!! Left a few comments (not all of them need to be addressed right now), but in general my only other concern is that we are migrating all of Conductor to Davis Design, which means we'll need to migrate these new Remixer components eventually, too. It may be easier to do so now before it goes to prod. But, for staging, it's not a concern -- I know we are anxious to start playing around with it and, as far as I can tell, the core functionality looks solid
| } | null>(null); | ||
| const user = useTypedSelector((state) => state.user); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
In a perfect world, we'd use useQuery from @tanstack/react-query here for consistency across the codebase, but it's certainly not a blocker for staging
| @@ -0,0 +1,158 @@ | |||
| import { Book, MasterCatalogV2Response } from "../../types"; | |||
|
|
|||
| export type Library = "bio" | "biz" | "chem" | "dev" | "eng" | "espanol" | "geo" | "human" | "k12" | "math" | "med" | "phys" | "socialsci" | "stats" | "workforce"; | |||
There was a problem hiding this comment.
Will be best to use the LIBRARIES constant in client\src\utils\constants.ts for a single source of truth, but this can be addressed in a subsequent PR
|
|
||
| const interval = setInterval(async () => { | ||
| try { | ||
| const res = await axios.get(`/commons/import-pressbooks/${jobID}`); |
There was a problem hiding this comment.
Recommend switching to useMutation from @tanstack/react-query in a subsequent PR for cleaner request state management and consistency
There was a problem hiding this comment.
Also will want to make sure we define all request paths in client\src\api.ts and consume them as, for example, api.getPressbooksImportJob()
| <Modal.Header>Import Book</Modal.Header> | ||
| <Modal.Content> | ||
| <p id="bookInstructions"> | ||
| This imports a book from a library into this Conductor project. |
There was a problem hiding this comment.
Recommend changing this to "This imports a book from an external source into this Conductor project" for clarity. The current text suggests we are doing more of a copy from LT library to LT library. We may also extend this modal in the future to start import jobs from sources other than PB so this language keeps it open
There was a problem hiding this comment.
Alternatively, we could add text like "You can import from the following sources: Pressbooks" and we can add to the list as needed
| control={control} | ||
| name="pbBookURL" | ||
| label="Book URL" | ||
| placeholder="Enter Book URL" |
There was a problem hiding this comment.
Recommend grabbing a good example pressbooks URL for the placeholder here
| router | ||
| .route("/remixer/:id/project") | ||
| .get( | ||
| authAPI.optionalVerifyRequest, |
There was a problem hiding this comment.
Should switch to authAPI.verifyRequest and authAPI.getUserAttributes for any Remixer endpoints -- we'll plan on everything remixer being behind auth for now
There was a problem hiding this comment.
You can ignore the rate limiting comments from CodeQL in this file -- these endpoints are rate-limited -- I'm not sure why it's not picking that up
| import * as cheerio from "cheerio"; | ||
| import { log } from "debug"; | ||
|
|
||
| export interface RemixerSubPage { |
There was a problem hiding this comment.
Recommend seeing if we can define this as the "root" interface in server/types/Remixer.ts and then extending the RemixerSubPageState interface in server\models\projectremixer.ts off of it for DRY principle and single source-of-truth
| return trimmedMessage.slice(0, 300); | ||
| }; | ||
|
|
||
| const buildConductorCookieHeader = (req: Request): string | undefined => { |
There was a problem hiding this comment.
Let's remove this entirely - the Conductor cookie headers won't mean anything to the libraries and the less we can mess with auth, the better
| } | ||
|
|
||
| export interface PublishOptions { | ||
| auth?: { username: string; password: string }; |
There was a problem hiding this comment.
Recommend removing the auth option entirely here - it doesn't look like we're actually making use of it anywhere publishBook is called and it will clean up the code and avoid future confusion
… components with Davis design system
…port and remixer.
…xer API endpoints
… components with Davis design system
| title: 1, | ||
| _id: 0, | ||
| }; | ||
| const project = await Project.findOne({ projectID: id }, projection); |
There was a problem hiding this comment.
NoSQL injection attack possible - critical severity
Query injection attacks are possible if users can pass objects instead of strings to query functions such as findOne.
By injecting query operators attackers can control the behavior of the query, allowing them to bypass access controls and extract unauthorized data. Consider the attack payload ?user_id[$ne]=5: if the user_id query parameter is passed to the query function without validation or casting its type, an attacker can pass {$ne: 5} instead of an integer to the query. {$ne: 5} uses the 'not equal to' operator to access data of other users.
While this vulnerability is known as NoSQL injection, relational databases (mysql, postgres) are also vulnerable to this attack if the query library offers a NoSQL-like API and supports string-typed query operators. Examples include prisma and sequelize versions prior to 4.12.0.
| const project = await Project.findOne({ projectID: id }, projection); | |
| const project = await Project.findOne({ projectID: { $eq: id } }, projection); |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Features added:
Consideration: package-lock.json was not added to avoid a merge conflict.