Bulk scan error handling #1126
Merged
Bulk scan error handling #1126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See documentation: https://docs.docker.com/compose/compose-file/04-version-and-name/#version-top-level-element-obsolete > The top-level version property is defined by the Compose > Specification for backward compatibility. It is only informative and > you'll receive a warning message that it is obsolete if used. I've removed this to prevent us from getting the following warning when docker compose commands are run: ``` WARN[0000] docker-compose.yaml: `version` is obsolete ```
Updates the asset scanner/fetcher to catch any exceptions the `requests` library raises when attempting to obtain the contents of various asset files. The previous behavior was to let those be raised. I've changed the `fetch_asset` function, the only place where we invoke `requests`, to return a string containing the text content of the response, rather than the response itself. This is a good change anyway, since it means the callers of `fetch_asset`, all of which only wanted the text content anyway, don't have to worry about the details of the `requests` library. One thing to note: if an error is caught, then the function returns the empty string. This is the simplest thing to do, but it means we're kind of passing over the error. If we want to flag it in the results somehow, then we will have to add some additional code and make decisions about how exactly we want that to work.
If an error occurs while conducting a bulk scan, skip the landing page currently being scanned and continue with the rest of the ones in the queue.
SaptakS
reviewed
May 29, 2024
| try: | ||
| current_result = perform_scan(entry.landing_page_url, permitted_domains) | ||
| except Exception: | ||
| continue |
There was a problem hiding this comment.
I am slightly worried about this failing too silently. Should we maybe have a warning of some sorts logging the error, in case there is some real error that we may need to debug?
SaptakS
approved these changes
May 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1125
Changes proposed in this pull request:
This PR adds some error handling to the scan process in two ways:
See commit messages for additional technical details.
Type of change
Testing
I've added unit tests for both changes, but it might be useful to try this locally with a bulk scan on the problematic landing page that inspired this issue.
Post-deployment actions
None.
Checklist
General checks
If you made changes to scanner
/api/v1/directoryreturns directory entries and scan results as expected