Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

[CE-1330] Escaping args #167

Merged
merged 2 commits into from
Feb 4, 2020
Merged

[CE-1330] Escaping args #167

merged 2 commits into from
Feb 4, 2020

Conversation

drazisil
Copy link
Contributor

@drazisil drazisil commented Feb 4, 2020

Sanitize gcov-root and ather args.

Copy link

@hootener hootener left a comment

Choose a reason for hiding this comment

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

To address:

  • should launch.json be .gitignored?
  • should we keep validator and perform a two step sanitization process? 1. pass through validator, 2. pass through sanitizeVar

@@ -0,0 +1,15 @@
{
Copy link

Choose a reason for hiding this comment

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

I'm not sure this should be committed to source since it's IDE specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this should not have been commited, good catch.

@@ -5,7 +5,6 @@ var urlgrey = require('urlgrey')
var jsYaml = require('js-yaml')
var walk = require('ignore-walk')
var execSync = require('child_process').execSync
var validator = require('validator')
Copy link

Choose a reason for hiding this comment

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

I'm unsure what benefits validator provides. But would it be worthwhile to keep it, and call it in your sanitizeVar function before the "&" removal step? Off the cuff, something like:

function sanitizeVar(arg) {
  arg = validator.escape(arg)
  return arg.replace(/&/g, '')
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validator was added with the last patch. It also escaped / which makes actual paths very unhappy.

@codecov
Copy link

codecov bot commented Feb 4, 2020

Codecov Report

Merging #167 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   90.14%   90.16%   +0.02%     
==========================================
  Files          23       23              
  Lines         355      356       +1     
  Branches       85       85              
==========================================
+ Hits          320      321       +1     
  Misses         35       35              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bac0787...2c7ec24. Read the comment docs.

@drazisil drazisil merged commit 02cf13d into master Feb 4, 2020
@drazisil drazisil deleted the escape-gcov-root branch February 4, 2020 15:41
@drazisil
Copy link
Contributor Author

drazisil commented Feb 4, 2020

@eddiemoore Can you please push a new release.

@drazisil
Copy link
Contributor Author

drazisil commented Feb 7, 2020

This has been published under https://www.npmjs.com/package/codecov/v/3.6.5

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants