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.

Sign up for GitHub

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

Jump to bottom

Play with code review #10

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

Play with code review #10

wants to merge 1 commit into from

Conversation

ferreiratiago
Copy link

No description provided.

ferreiratiago
ferreiratiago commented Aug 7, 2024
View reviewed changes
Copy link
Author

@ferreiratiago ferreiratiago left a comment •
edited by andygongea
Loading

Choose a reason for hiding this comment

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

⚠️ Snyk found 5 issues: 1 in the modified code and 4 outside it

1 issue in the modified code

  •   Use of Hardcoded Credentials
    • Path: app.js#17

4 issues outside the modified code

app.js Show resolved Hide resolved
ferreiratiago
ferreiratiago commented Aug 7, 2024
View reviewed changes
Copy link
Author

@ferreiratiago ferreiratiago left a comment •
edited
Loading

Choose a reason for hiding this comment

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

[OUTSIDE THE DIFF]

[OPTION 1]

Snyk has found 3 issues outside the changed files:

[OPTION 2]

Snyk has found 5 issues outside the changed files:

app.js
  •   Use of Hardcoded Credentials on line 17

ferreiratiago
ferreiratiago commented Aug 22, 2024
View reviewed changes
Copy link
Author

@ferreiratiago ferreiratiago left a comment •
edited by andygongea
Loading

Choose a reason for hiding this comment

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

⚠️ Snyk found 5 issues: 1 in the modified code and 2 outside it

1 issue in the modified code

Severity Issue Resolved
NoSQL Injection

2 issues outside the modified code

Severity Issue Resolved
NoSQL Injection
Use of Hardcoded Credentials ✅

app.js
@@ -56,7 +56,7 @@ app.post('/login', routes.loginHandler);
app.get('/admin', routes.isLoggedIn, routes.admin);
app.get('/account_details', routes.isLoggedIn, routes.get_account_details);
app.post('/account_details', routes.isLoggedIn, routes.save_account_details);
app.get('/logout', routes.logout);
app.get('/logout/now', routes.logout);
app.post('/create', routes.create);
app.get('/destroy/:id', routes.destroy);
app.get('/edit/:id', routes.edit);
Copy link
Author

@ferreiratiago ferreiratiago Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  NoSQL Injection

Unsanitized input from the HTTP request body flows into find, where it is used in an NoSQL query. This may result in an NoSQL Injection vulnerability.

Line 81 | Priority score 555 | CWE-319

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

@ferreiratiago
Copy link
Author

ferreiratiago commented Aug 22, 2024 •
edited
Loading

⚠️ Snyk also found 2 vulnerabilities outside the modified code

----------------------------------------------- OPTION 1 -----------------------------------------------

Severity Issue
NoSQL Injection
- Path: utils.js#L10
Use of Hardcoded Credentials
- Path: public/css/screen.css#L23

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

----------------------------------------------- OPTION 2 -----------------------------------------------

Severity Issue
NoSQL Injection

tiago_pr_playground/utils.js

Line 10 in 433c1a3

var src_len = src.length;
Use of Hardcoded Credentials

tiago_pr_playground/public/css/screen.css

Line 23 in 433c1a3

/* line 20, ../../../../../Users/fred/.rvm/gems/ruby-1.9.3-p0/gems/compass-0.12.1/frameworks/compass/stylesheets/compass/reset/_utilities.scss */
Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

----------------------------------------------- OPTION 3 -----------------------------------------------

  NoSQL Injection

Unsanitized input from the HTTP request body flows into find, where it is used in an NoSQL query. This may result in an NoSQL Injection vulnerability.

Path: utils.js#L10

CWE-352 | Priority score 560

  Use of Hardcoded Credentials

http.createServer uses HTTP which is an insecure protocol and should not be used in code due to cleartext transmission of information. Data in cleartext in a communication channel can be sniffed by unauthorized actors. Consider using the https module instead.

Path: public/css/screen.css#L23

CWE-352 | Priority score 560

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

----------------------------------------------- OPTION 4 -----------------------------------------------

  NoSQL Injection

Unsanitized input from the HTTP request body flows into find, where it is used in an NoSQL query. This may result in an NoSQL Injection vulnerability.

Path:

tiago_pr_playground/utils.js

Line 10 in 433c1a3

var src_len = src.length;

CWE-352 | Priority score 560

  Use of Hardcoded Credentials

http.createServer uses HTTP which is an insecure protocol and should not be used in code due to cleartext transmission of information. Data in cleartext in a communication channel can be sniffed by unauthorized actors. Consider using the https module instead.

Path:

tiago_pr_playground/public/css/screen.css

Line 23 in 433c1a3

/* line 20, ../../../../../Users/fred/.rvm/gems/ruby-1.9.3-p0/gems/compass-0.12.1/frameworks/compass/stylesheets/compass/reset/_utilities.scss */

CWE-352 | Priority score 560

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

----------------------------------------------- OPTION 5 -----------------------------------------------

tiago_pr_playground/utils.js

Line 10 in 433c1a3

var src_len = src.length;

  NoSQL Injection

Unsanitized input from the HTTP request body flows into find, where it is used in an NoSQL query. This may result in an NoSQL Injection vulnerability.

CWE-352 | Priority score 560

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

----------------------------------------------- OPTION 6 -----------------------------------------------

  Use of Hardcoded Credentials

http.createServer uses HTTP which is an insecure protocol and should not be used in code due to cleartext transmission of information. Data in cleartext in a communication channel can be sniffed by unauthorized actors. Consider using the https module instead.

public/css/screen.css#L23 | CWE-352 | Priority score 560

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

@ferreiratiago
Copy link
Author

----------------------------------------------- OPTION 1 -----------------------------------------------
USE STRIKETHROUGH

⚠️ Snyk found 3 issues: 1 in the modified code and 2 outside it

1 issue in the modified code (All resolved)

  •   Use of Hardcoded Credentials

2 issues outside the modified code (1 resolved)

  •   Use of Hardcoded Credentials
  •   Use of Hardcoded Credentials

Wouldn't this make unresolved comments info hard to read?

----------------------------------------------- OPTION 2.2 -----------------------------------------------
DELETE

⚠️ Snyk found 1 issue outside the modified code

1 issue outside the modified code

  •   Use of Hardcoded Credentials

WHEN ALL COMMENTS RESOLVED

✅ Snyk hasn't found any vulnerabilities

Wouldn't this be confusing because of the inline comments (even if resolved)?

@andygongea
Copy link
Collaborator

⚠️ Snyk found 5 issues: 2 in the modified code and 2 outside of it

Severity Issue
NoSQL Injection
- Path: utils.js#L10
Use of Hardcoded Credentials
- Path: public/css/screen.css#L23

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

Here are the 2 issues within the modified code:

andygongea
andygongea reviewed Aug 29, 2024
View reviewed changes
Copy link
Collaborator

@andygongea andygongea left a comment

Choose a reason for hiding this comment

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

⚠️ Snyk found 5 issues: 2 in the modified code and 2 outside of it

Severity Issue
NoSQL Injection
- Path: utils.js#L10
Use of Hardcoded Credentials
- Path: public/css/screen.css#L23

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

Here are the 2 issues within the modified code:

app.js
@@ -56,7 +56,7 @@ app.post('/login', routes.loginHandler);
app.get('/admin', routes.isLoggedIn, routes.admin);
app.get('/account_details', routes.isLoggedIn, routes.get_account_details);
app.post('/account_details', routes.isLoggedIn, routes.save_account_details);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  NoSQL Injection

Unsanitized input from the HTTP request body flows into find, where it is used in an NoSQL query. This may result in an NoSQL Injection vulnerability.

Line 81 | Priority score 555 | CWE-319

Data flow: 3 steps

tiago_pr_playground/app.js

Line 52 in 855a1fa

app.use(routes.current_user);

tiago_pr_playground/app.js

Line 75 in 855a1fa

marked.setOptions({ sanitize: true });

tiago_pr_playground/app.js

Line 80 in 855a1fa

app.use(errorHandler());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andygongea andygongea andygongea left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ferreiratiago @andygongea