add --update-db option to update local MAC address prefix database#413
Merged
Conversation
…om IEEE registries
Reviewer's GuideIntroduce a new Sequence diagram for --update-db option triggering database updatesequenceDiagram
actor User
participant "wifite.py"
participant "Configuration"
participant "DBUpdater"
User->>"wifite.py": Run with --update-db
"wifite.py"->>"Configuration": load_from_arguments()
"wifite.py"->>"DBUpdater": run()
"DBUpdater"->>"Configuration": initialize(False)
"DBUpdater"->>"DBUpdater": is_up_to_date(filename)
alt Database is up to date
"DBUpdater"-->>"wifite.py": Print up-to-date message
else Database needs update
"DBUpdater"->>"DBUpdater": update_all(filename, verbose)
"DBUpdater"-->>"wifite.py": Print completion message
end
Class diagram for new DBUpdater class and Configuration changesclassDiagram
class Configuration {
+update_db: bool
+db_filename: str
+initialize(load_interface: bool)
+load_from_arguments()
+validate()
}
class DBUpdater {
+SOURCES: dict
+DEFAULT_FILENAME: str
+run()
+update_all(filename: str, verbose: bool): int
+fetch_csv(url: str, verbose: bool): str
+parse_and_write_csv(csv_content: str, outfile, key: str, verbose: bool): int
+is_up_to_date(filename: str): (bool, str)
}
Configuration <.. DBUpdater : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Blocking issues:
- Detected a 'requests' call without a timeout set. By default, 'requests' calls wait until the connection is closed. This means a 'requests' call without a timeout will hang the program if a response is never received. Consider setting a timeout for all 'requests'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `wifite/util/dbupdater.py:34` </location>
<code_context>
+ @classmethod
+ def run(cls):
+
+ Configuration.initialize(False)
+
+ filename = Configuration.db_filename
</code_context>
<issue_to_address>
**question (bug_risk):** Re-initializing Configuration may override user-supplied values.
Review whether Configuration.initialize(False) is needed here, as it may reset db_filename or verbose set by the caller. Preserving existing values may be preferable.
</issue_to_address>
### Comment 2
<location> `wifite/util/dbupdater.py:83-91` </location>
<code_context>
+ headers = {"User-Agent": "Mozilla/5.0 (compatible; DBUpdater/1.0)"}
+ if verbose:
+ Color.pl(' → Fetching %s' % url)
+ response = requests.get(url, headers=headers, timeout=30)
+ if not response.ok:
+ raise RuntimeError(f"Failed to fetch {url}: {response.status_code} {response.reason}")
</code_context>
<issue_to_address>
**suggestion:** No retry logic for transient network failures.
Consider implementing retries or handling transient exceptions to prevent updates from failing due to temporary network issues.
```suggestion
import time
from requests.exceptions import ConnectionError, Timeout, RequestException
headers = {"User-Agent": "Mozilla/5.0 (compatible; DBUpdater/1.0)"}
max_retries = 3
backoff = 5 # seconds
for attempt in range(1, max_retries + 1):
if verbose:
Color.pl(f' → Fetching {url} (attempt {attempt}/{max_retries})')
try:
response = requests.get(url, headers=headers, timeout=30)
if not response.ok:
raise RuntimeError(f"Failed to fetch {url}: {response.status_code} {response.reason}")
if len(response.content) == 0:
raise RuntimeError(f"Empty response from {url}")
return response.text
except (ConnectionError, Timeout) as e:
if attempt == max_retries:
raise RuntimeError(f"Failed to fetch {url} after {max_retries} attempts: {e}")
if verbose:
Color.pl(f" → Network error: {e}. Retrying in {backoff} seconds...")
time.sleep(backoff)
except RequestException as e:
raise RuntimeError(f"Failed to fetch {url}: {e}")
```
</issue_to_address>
### Comment 3
<location> `wifite/util/dbupdater.py:41` </location>
<code_context>
+ verbose = args.v
+
+ # Delete old file if exists
+ if os.path.exists(filename):
+ if verbose:
+ print(f"Deleting existing {filename}")
</code_context>
<issue_to_address>
**issue (bug_risk):** Deleting the existing database file before confirming successful download may risk data loss.
If the update fails post-deletion, the local database is unrecoverable. Write to a temp file and replace the original only after a successful update to prevent this.
</issue_to_address>
### Comment 4
<location> `wifite/util/dbupdater.py:17` </location>
<code_context>
+# TODO: Add logging support, dry-run, and a --no-download option for offline parsing
+
+
+class DBUpdater:
+ """Updates a local database of MAC address prefixes to vendor names from IEEE registries.
+ """
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the code to use module-level functions instead of a class to simplify structure and clarify data flow.
Here’s one way to flatten the abstraction by turning all of your `@classmethod`s into module-level functions. You keep 100% of the behavior, but drop the unnecessary `DBUpdater` wrapper (and its boilerplate).
1) Pull your constants out to module‐scope:
```python
SOURCES = {
"OUI": "https://standards-oui.ieee.org/oui/oui.csv",
# …
}
DEFAULT_FILENAME = "ieee-oui.txt"
```
2) Turn each `@classmethod` into a top-level function:
```python
def is_up_to_date(filename: str) -> (bool, str):
mtime = os.path.getmtime(filename)
last = datetime.fromtimestamp(mtime).strftime("%Y-%m-%d %H:%M:%S")
age = datetime.now().timestamp() - mtime
return (age < 7 * 24 * 3600), last
def fetch_csv(url: str, verbose: bool = False) -> str:
headers = {"User-Agent": "DBUpdater/1.0"}
if verbose:
Color.pl(f" → Fetching {url}")
r = requests.get(url, headers=headers, timeout=30)
if not r.ok or not r.content:
raise RuntimeError(f"Failed to fetch {url}: {r.status_code} {r.reason}")
return r.text
def parse_and_write_csv(txt: str, out, key: str, verbose: bool = False) -> int:
reader = csv.DictReader(txt.splitlines())
out.write(f"\n# Start of {key}\n")
cnt = 0
for row in reader:
mac = row.get("Assignment") or row.get("Registry") or ""
vendor = (row.get("Organization Name") or row.get("Organization") or "").strip()
if mac and vendor:
out.write(f"{mac}\t{vendor}\n")
cnt += 1
out.write(f"# End of {key}. {cnt} entries.\n")
Color.p(f" Wrote {cnt} entries from {key}")
return cnt
```
3) Rewrite your main loop as a single `run()`:
```python
def update_all(filename: str, verbose: bool) -> int:
total = 0
with open(filename, "w", encoding="utf-8") as out:
out.write(f"# Generated {datetime.now():%Y-%m-%d %H:%M:%S}\n")
for key, url in SOURCES.items():
Color.pl(f"\n{{+}} Processing {key} from {url}")
try:
txt = fetch_csv(url, verbose)
total += parse_and_write_csv(txt, out, key, verbose)
except Exception as e:
print(f"Error processing {key}: {e}", file=sys.stderr)
return total
def run():
Configuration.initialize(False)
fn, vb = Configuration.db_filename, bool(Configuration.verbose)
if os.path.exists(fn):
up, last = is_up_to_date(fn)
if up:
return Color.pl(f"Database up-to-date ({last})")
if vb:
Color.pl(f"Deleting old {fn}")
os.remove(fn)
try:
cnt = update_all(fn, vb)
except KeyboardInterrupt:
return Color.pl("Interrupted by user")
Color.pl(f"Done – total entries: {cnt}")
if __name__ == "__main__":
run()
```
This removes one level of nesting, clarifies data flow, and avoids a “no-state” class.
</issue_to_address>
### Comment 5
<location> `tools/fetch_oui.py:25` </location>
<code_context>
response = requests.get(url, headers=headers)
</code_context>
<issue_to_address>
**security (python.requests.best-practice.use-timeout):** Detected a 'requests' call without a timeout set. By default, 'requests' calls wait until the connection is closed. This means a 'requests' call without a timeout will hang the program if a response is never received. Consider setting a timeout for all 'requests'.
```suggestion
response = requests.get(url, headers=headers, timeout=30)
```
*Source: opengrep*
</issue_to_address>
### Comment 6
<location> `wifite/util/dbupdater.py:85` </location>
<code_context>
@classmethod
def fetch_csv(cls, url: str, verbose: bool = False) -> str:
"""Download CSV content (boilerplate; uses requests)."""
headers = {"User-Agent": "Mozilla/5.0 (compatible; DBUpdater/1.0)"}
if verbose:
Color.pl(' → Fetching %s' % url)
response = requests.get(url, headers=headers, timeout=30)
if not response.ok:
raise RuntimeError(f"Failed to fetch {url}: {response.status_code} {response.reason}")
if len(response.content) == 0:
raise RuntimeError(f"Empty response from {url}")
return response.text
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace interpolated string formatting with f-string ([`replace-interpolation-with-fstring`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/replace-interpolation-with-fstring/))
```suggestion
Color.pl(f' → Fetching {url}')
```
</issue_to_address>
### Comment 7
<location> `wifite/util/dbupdater.py:111-116` </location>
<code_context>
def is_up_to_date(filename: str) -> bool:
mtime = os.path.getmtime(filename)
last_update = datetime.fromtimestamp(mtime).strftime("%Y-%m-%d %H:%M:%S")
age_seconds = datetime.now().timestamp() - mtime
return age_seconds < (7 * 24 * 3600), last_update #if file is older than 7 days it is not up to date
</code_context>
<issue_to_address>
**issue (code-quality):** The first argument to instance methods should be `self` ([`instance-method-first-arg-name`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/instance-method-first-arg-name/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Added timeout Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Created
--update-dband added a python version oftools/fetch-oui.I dont mind if you change any of the names that I have assigned (like
--update-dbor the classdbupdater.py)