Skip to content

Fix inconsistent port type in scan event records [ Fixes #1649] - #1652

Open
Aarush289 wants to merge 6 commits into
OWASP:masterfrom
Aarush289:port_storage_data_type_fix
Open

Fix inconsistent port type in scan event records [ Fixes #1649]#1652
Aarush289 wants to merge 6 commits into
OWASP:masterfrom
Aarush289:port_storage_data_type_fix

Conversation

@Aarush289

Copy link
Copy Markdown
Contributor

Proposed change

This PR fixes #1649
The following picture shows that before fix the port as stored as string while after the fix it was stored as int.
image

Type of change

  • New core framework functionality
  • Bugfix (non-breaking change that fixes an issue)
  • Code refactoring without any functionality changes
  • New or existing module/payload change
  • Documentation/localization improvement
  • Test coverage improvement
  • Dependency upgrade
  • Other improvement (best practice, cleanup, optimization, etc)

Checklist

  • I've followed the contributing guidelines
  • I've digitally signed all my commits in this PR
  • I've run make pre-commit and confirm it didn't generate any warnings/changes
  • I've run make test and I confirm all tests passed locally
  • I've added/updated any relevant documentation in the docs/ folder
  • I've linked this PR with an open issue
  • I've tested and verified that my code works as intended and resolves the issue as described
  • I've attached screenshots demonstrating that my code works as intended (if applicable)
  • I've checked all other open PRs to avoid submitting duplicate work
  • I confirm that the code and comments in this PR are not direct unreviewed outputs of AI
  • I confirm that I am the Sole Responsible Author for every line of code, comment, and design decision

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • Bug Fixes
    • Improved port detection by prioritizing configured values and extracting ports from URLs when needed.
    • Standardized numeric port values across event and database logs.
    • Improved handling of nonnumeric port values to prevent inconsistent logging.
    • Ensured comparison reports treat equivalent numeric ports consistently, regardless of whether they are entered as numbers or digit-only text.

Walkthrough

process_conditions selects a port from event fields or a numeric URL port and normalizes numeric strings. Temporary-event and database log records use this value. Comparison tuples also normalize digit-only string ports.

Changes

Port normalization

Layer / File(s) Summary
Port extraction and storage
nettacker/core/lib/base.py
process_conditions prioritizes ports, then port, then a numeric URL port. Numeric strings become integers, while other strings remain unchanged. Temporary-event and database log records use the shared value.
Comparison port normalization
nettacker/core/graph.py
get_modules_ports converts digit-only string ports to integers before returning comparison tuples.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • OWASP/Nettacker#1648: The changes extend port derivation in process_conditions and add port normalization to comparison reporting.

Suggested reviewers: arkid15r

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the port type fix and references the linked issue.
Description check ✅ Passed The description explains that the bugfix normalizes ports and includes the related issue and validation details.
Linked Issues check ✅ Passed The changes normalize numeric ports before storage and comparison, satisfying issue #1649.
Out of Scope Changes check ✅ Passed The changes in base.py and graph.py directly support consistent numeric port handling and stay within issue #1649.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@nettacker/core/lib/base.py`:
- Around line 136-140: Replace the duplicated URL port extraction expressions in
both log branches with one shared helper using urllib.parse.urlsplit(...).port;
have it accept the URL string, catch ValueError, and return an empty string when
no valid port is present. Update both event-processing locations to reuse this
helper instead of split-based parsing.
- Around line 133-135: Normalize the selected port value in process_conditions
before constructing the database record: after choosing event.get("ports"),
event.get("port"), or the fallback, convert string ports such as "443" to the
expected numeric representation. Apply the same normalization to the
corresponding port-selection logic referenced around the second location, while
preserving existing precedence and fallback behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0d33ef86-3575-40fd-9f6a-b52175ea3bb7

📥 Commits

Reviewing files that changed from the base of the PR and between 9fa9645 and aaa9a66.

📒 Files selected for processing (1)
  • nettacker/core/lib/base.py

Comment thread nettacker/core/lib/base.py Outdated
Comment thread nettacker/core/lib/base.py Outdated
@Aarush289
Aarush289 marked this pull request as draft August 6, 2026 07:41
@Aarush289
Aarush289 marked this pull request as ready for review August 6, 2026 18:33
@securestep9

Copy link
Copy Markdown
Collaborator

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9278cdf4fa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread nettacker/core/lib/base.py
Comment thread nettacker/core/lib/base.py
Comment thread nettacker/core/lib/base.py
Comment thread nettacker/core/lib/base.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@nettacker/core/lib/base.py`:
- Around line 136-138: Replace the isdigit()-based conversion in
nettacker/core/lib/base.py lines 136-138 with a guarded direct int() attempt,
preserving the original string when conversion raises ValueError before storing
the port. Apply the same normalization change in nettacker/core/graph.py lines
352-353 before creating compare tuples, so Unicode digit strings that int()
rejects remain unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1c95ff59-895c-4702-b2a8-8033bef7ceb4

📥 Commits

Reviewing files that changed from the base of the PR and between 3fcedc1 and 7b83760.

📒 Files selected for processing (2)
  • nettacker/core/graph.py
  • nettacker/core/lib/base.py

Comment thread nettacker/core/lib/base.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consistently store port as the same data type (int)

2 participants