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

fix: Removed STIG filename filters #260

Merged
merged 3 commits into from
Apr 21, 2021
Merged

fix: Removed STIG filename filters #260

merged 3 commits into from
Apr 21, 2021

Conversation

cd-rite
Copy link
Collaborator

@cd-rite cd-rite commented Apr 18, 2021

Removed attempts to filter STIG processing based on filename (Aside from requiring a xml/XML extension), since they do not seem to follow any reliable convention. Restricted error responses to just parser error message (removed stack trace portion so it does not show up in user's import log).

fixes: #235

cd-rite added 2 commits April 18, 2021 16:03
@cd-rite
Removed attempts to filter STIG processing based on filename, since t…
dc395a1 
…hey do not seem to follow any reliable convention. Restricted error responses to just parser error message (removed stack trace portion so it does not show up in user's import log).
@cd-rite
Merge branch 'NUWCDIVNPTgh-235-stig-import-filters' into upstream-merge
e135b96
csmig
csmig reviewed Apr 19, 2021
View reviewed changes
api/source/utils/fetchStigs.js Outdated
@@ -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.

clients/extjs/js/stigmanUtils.js
@@ -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

@csmig csmig merged commit 80aa541 into NUWCDIVNPT:main Apr 21, 2021
cd-rite added a commit to cd-rite/stig-manager that referenced this pull request May 6, 2021
@cd-rite @csmig @dependabot
Tests 5 5 (#20)
0c04320 
* feat: Collection export management (NUWCDIVNPT#169)

* fix: case-insensitive filename matching (NUWCDIVNPT#192)

* fix: Improved output when importing STIG XML (NUWCDIVNPT#192)

* doc: Show Export CKLs in screenshots

* chore: Bump release to 1.0.0-beta.22

* adjust path to docker readme (NUWCDIVNPT#196)

* doc: Added some documentation about new .ckl archive export feature. (NUWCDIVNPT#203)

* removed some todos

* stig archive export feature

* feat: name-match params and duplicate handling (NUWCDIVNPT#204)

* feat: case-sensitive collation for benchmarkId in MySQL (NUWCDIVNPT#206)

* Common tasks elaboration, other edits (NUWCDIVNPT#208)

* feat: progress bar styling (NUWCDIVNPT#209)

* feat: UI shows collectionId (NUWCDIVNPT#210)

* fix: remove hard-coded reference to schema (NUWCDIVNPT#211)

* chore: Bump release to 1.0.0-beta.23

* fix: reduce deadlock potential (NUWCDIVNPT#216)

* api links (NUWCDIVNPT#219)

* build(deps): bump y18n from 3.2.1 to 3.2.2 in /api/source

Bumps [y18n](https://github.com/yargs/y18n) from 3.2.1 to 3.2.2.
- [Release notes](https://github.com/yargs/y18n/releases)
- [Changelog](https://github.com/yargs/y18n/blob/master/CHANGELOG.md)
- [Commits](https://github.com/yargs/y18n/commits)

Signed-off-by: dependabot[bot] <support@github.com>

* doc: Added a little more about .ckl and data handling (NUWCDIVNPT#223)

* just rst changes

* sphinx generation

* fix: Exports on multiple reports (NUWCDIVNPT#224)

* Multiple fixes and features (NUWCDIVNPT#225)

* feat: return 401 when no token provided

* feat: home-widget-bwrap

* fix: collectionReview buttons

* fix: deadlock prevention status updates

* chore: Bump release to 1.0.0-beta.24

* fix: fetch STIG/SCAP if configured at bootstrap (NUWCDIVNPT#227)

Fixes NUWCDIVNPT#213

* Multiple fix and features (NUWCDIVNPT#228)

* feat: CKL parser retains empty comments
* feat: enable accept when selections include accept
* fix: review form button behaviors, etc. (NUWCDIVNPT#215)

* chore: remove unused oracledb dependency (NUWCDIVNPT#229)

* chore: remove unused oracledb dependency

* Remove unused require

* chore: Bump release to 1.0.0-beta.25

* feat: Manage Assets -> multi-delete (NUWCDIVNPT#232), columns (NUWCDIVNPT#236)

* fix: include promisfied confirm (NUWCDIVNPT#237)

* build(deps): bump urllib3 from 1.26.3 to 1.26.4 in /docs (NUWCDIVNPT#238)

Bumps [urllib3](https://github.com/urllib3/urllib3) from 1.26.3 to 1.26.4.
- [Release notes](https://github.com/urllib3/urllib3/releases)
- [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst)
- [Commits](urllib3/urllib3@1.26.3...1.26.4)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* doc: updates regarding ckl -> stigman field mappings, clients folder when running from source (NUWCDIVNPT#241)

doc: updates regarding ckl -> stigman field mappings, clients folder when running from source

* feat: Tooltips for Review labels and headers (NUWCDIVNPT#240) (NUWCDIVNPT#242)

* feat: mercury-medium color is more blue (NUWCDIVNPT#243)

* fix: sticky bit for world-writable dirs created by npm (NUWCDIVNPT#245)

* chore: Bump release to 1.0.0-beta.26

* fix: increased length of asset name,ip,mac,fqdn and allow more nulls  (NUWCDIVNPT#251)

* added maxLength properties of 255 for ip, mac, asset name, and fqdn; added nullable:true for collection description properties

* removed vtype specification for ip address, as we will no longer be validating ip address field.

* Added migration file to alter varchar sizes for asset ip, mac, and name

* fix: batch import continues on error, refreshes grids (NUWCDIVNPT#252)

* feat: Ext.LoadMask looks for store.smMaskDelay (NUWCDIVNPT#254)

* chore: Bump release to 1.0.0-beta.27

* fix: log servicename if present (NUWCDIVNPT#198)

* fix: Attach => Assign STIG (NUWCDIVNPT#118)

* fix: response schema for /opt/configuration (NUWCDIVNPT#147)

* fix: create date is not ISO8601 UTC (NUWCDIVNPT#189)

* fix: handle property chains with hyphens (NUWCDIVNPT#257)

* fix: cast userId as char (NUWCDIVNPT#249)

* feat: format roles claim with bracket notation and optional chaining (NUWCDIVNPT#190)

* fix: SET NAME to utf8mb4 encoding (NUWCDIVNPT#262)

* fix: New/Delete => Assign/Unassign (NUWCDIVNPT#261)

* fix: New/Delete => Assign/Unassign (NUWCDIVNPT#118)

* dump docker logs on failure or cancellation

* fix: Filter members only on .xml extension  (NUWCDIVNPT#260)

* Removed attempts to filter STIG processing based on filename, since they do not seem to follow any reliable convention. Restricted error responses to just parser error message (removed stack trace portion so it does not show up in user's import log).

* added specific try/catch blocks around xml parsing

* fix NUWCDIVNPT#264: Display feedback for rejected reviews (NUWCDIVNPT#265)

* chore: Bump release to 1.0.0-beta.28

* fix NUWCDIVNPT#256: CKL site/instance handling; UI refactor (NUWCDIVNPT#268)

* chore: Bump release to 1.0.0-beta.29

* ironbank => development sign+image

* fix NUWCDIVNPT#266: sanitize exported filenames (NUWCDIVNPT#273)

* fix NUWCDIVNPT#270: ROLE element default value 'None' (NUWCDIVNPT#272)

* chore: Bump release to 1.0.0-beta.30

* fix NUWCDIVNPT#276: remove reference to database 'stigman'

* chore: remove obsolete docker dir (NUWCDIVNPT#278)

* Docs: Added default_group to prevent guid generation, removed doctrees, added a bit of info to Contributing doc. (NUWCDIVNPT#281)

* added default_group for images to stop guid generation
* removed doctrees
* added doctrees to .gitignore
*  added a couple paragraphs to contributing doc

* Endpoint updates (NUWCDIVNPT#284)

* feat: GET /assets metadata parameter

* feat: PUT /assets/{assetId}/stigs/{benchmarkId}

* tests match OpenAPI spec

* fix NUWCDIVNPT#145: Review vetting for all users (NUWCDIVNPT#285)

* fix NUWCDIVNPT#145: Review vetting for lvl1 users

* lvl1 cross-boundary tests, xccdf test file added, workflow updated to run new folder. Removed extra folders from Collection

* refactor adminStats, scc parser, tests, workflow

Co-authored-by: cd-rite <github-rite@notdoneyet.net>

* feat: Drag from Review History (NUWCDIVNPT#288)

* fix NUWCDIVNPT#275: handle rule-result without check (NUWCDIVNPT#290)

* fix NUWCDIVNPT#275: handle rule-result without check

* asset properties and benchmarkId check

* chore: Bump release to 1.0.0-beta.31

* checks for asset with no assigned STIGs, changed lvl1 checks to look for existing rule to which it does not have access (as opposed to non-existent rule)

* marked tests as continue-on-error so remaining tests would still run.

Co-authored-by: csmig <carlsmigielski@gmail.com>
Co-authored-by: csmig <33138761+csmig@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@cd-rite cd-rite deleted the upstream-merge branch August 2, 2021 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csmig csmig csmig 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.

BUG: Splunk STIG .zip won't Import
2 participants
@cd-rite @csmig