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

fix: Removed STIG filename filters #260

Merged
merged 3 commits into from
Apr 21, 2021
Merged

fix: Removed STIG filename filters #260

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc395a1
Removed attempts to filter STIG processing based on filename, since t…
cd-rite Apr 18, 2021
e135b96
Merge branch 'gh-235-stig-import-filters' into upstream-merge
cd-rite Apr 18, 2021
0c6a08e
added specific try/catch blocks around xml parsing
cd-rite Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/source/controllers/STIG.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const STIG = require(`../service/${config.database.type}/STIGService`)
module.exports.importManualBenchmark = async function importManualBenchmark (req, res, next) {
try {
let extension = req.file.originalname.substring(req.file.originalname.lastIndexOf(".")+1)
if (extension != 'xml') {
if (extension.toLowerCase() != 'xml') {
throw (writer.respondWithCode ( 400, {message: `File extension .${extension} not supported`} ))
}
let xmlData = req.file.buffer
Expand All @@ -23,7 +23,7 @@ module.exports.importManualBenchmark = async function importManualBenchmark (req
writer.writeJson(res, response)
}
catch(err) {
writer.writeJson(res, err)
writer.writeJson(res, {message: err.message})
}
}

Expand Down
8 changes: 4 additions & 4 deletions api/source/utils/fetchStigs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const scapURL = 'https://public.cyber.mil/stigs/scap/'
const stigMatchString = '<a href="(https://dl.dod.cyber.mil/wp-content/uploads/stigs/zip/.*)" target=.*'
const scapMatchString = '<a href="(https://dl.dod.cyber.mil/wp-content/uploads/stigs/zip/.*enchmark.zip)" target=.*'

// let localCompilationFile = '/STIGs/U_SRG-STIG_Library_2021_01v2.zip'
// let localCompilationFile = 'E:/STIGs/U_SRG-STIG_Library_2021_01v2.zip'


exports.fetchCompilation = async function fetchCompilation() {
Expand Down Expand Up @@ -72,7 +72,7 @@ exports.fetchScap = async function fetchScap() {
// await processZip(data)
// }
// catch (e) {
// throw (e)
// console.log(e)
// }
// }

Expand All @@ -82,7 +82,7 @@ async function processZip (f) {

let contents = await parentZip.loadAsync(f)
let fns = Object.keys(contents.files)
let xmlMembers = fns.filter( fn => fn.endsWith('xccdf.xml') || fn.endsWith('Benchmark.xml') )
let xmlMembers = fns.filter( fn => fn.toLowerCase().endsWith('.xml'))
let zipMembers = fns.filter( fn => fn.endsWith('.zip') )
for (let x=0,l=xmlMembers.length; x<l; x++) {
let xml = xmlMembers[x]
Expand Down Expand Up @@ -111,7 +111,7 @@ async function processZip (f) {
}
}
catch (e) {
throw (e)
Copy link
Member

Choose a reason for hiding this comment

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

We do need to throw an error here so the caller knows something went wrong. I think this try block covers too many operations, some of which should cause an immediate catch/throw and some that should not. I believe you're trying to avoid bombing out completely if a particular member fails parsing. The answer is to use nested try blocks. Maybe surround line 110 with try/catch and handle the error there without throwing to the outer block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added specific try/catch blocks to handle parsing errors.

console.log(`Caught error: ${e}`)
}

}
4 changes: 2 additions & 2 deletions clients/extjs/js/stigmanUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1646,7 +1646,7 @@ function uploadStigs(n) {
let input = document.getElementById("form-file-file")
let file = input.files[0]
let extension = file.name.substring(file.name.lastIndexOf(".")+1)
if (extension === 'xml') {
if (extension.toLowerCase() === 'xml') {
let formEl = fp.getForm().getEl().dom
let formData = new FormData(formEl)
formData.set('replace', 'true')
Expand Down Expand Up @@ -1691,7 +1691,7 @@ function uploadStigs(n) {

let contents = await parentZip.loadAsync(f)
let fns = Object.keys(contents.files)
let xmlMembers = fns.filter( fn => fn.toLowerCase().endsWith('xccdf.xml') || fn.toLowerCase().endsWith('benchmark.xml') )
let xmlMembers = fns.filter( fn => fn.toLowerCase().endsWith('.xml'))
Copy link
Member

Choose a reason for hiding this comment

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

As you said, this ends up throwing all XML members to the API and makes the API sort things out. Instead of just throwing everything over the wall, it might be more efficient (for the API at least) and a better user experience (in the client) to implement basic vetting of the file in the client. Only send the API something we reasonably think is valid.

Maybe we can iterate to that. I haven't run this client code yet, maybe this is acceptable for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'll leave this as something we will iterate too if needed

let zipMembers = fns.filter( fn => fn.toLowerCase().endsWith('.zip') )
for (let x=0,l=xmlMembers.length; x<l; x++) {
let xml = xmlMembers[x]
Expand Down